Skip to content

Initial commit - move scripts into new release/ dir#690

Closed
btoll wants to merge 3 commits intoalgorand:masterfrom
btoll:build_release
Closed

Initial commit - move scripts into new release/ dir#690
btoll wants to merge 3 commits intoalgorand:masterfrom
btoll:build_release

Conversation

@btoll
Copy link
Copy Markdown

@btoll btoll commented Jan 6, 2020

See the README

Comment thread scripts/release/build.sh Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why ? doesn't make build creates that automatically ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have to check with Brian to see if there's a reason to build this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this shouldn't be necessary, here and a couple other places, make build should take care of building libsodium, but I guess at one point in the past this worked around a finicky build. Probably we should try without it and see if we don't need it and maybe this time we can take the time to fix it right, or comment here why it's a good workaround

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brian is correct. At some point in the past it was needed.
Today, assuming that you're building on a clean environment every time, there shouldn't be a need to do that. ( but please do correct me if I'm wrong )

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've found that if I don't include this right before we make the rpm package that it won't build.

For example, if I comment out the code here:

# definitely rebuild libsodium which could link to external C libraries
#if [ -f ${REPO_DIR}/crypto/libsodium-fork/Makefile ]; then
#    (cd ${REPO_DIR}/crypto/libsodium-fork && make distclean)
#fi
#rm -rf ${REPO_DIR}/crypto/lib
#make crypto/lib/libsodium.a

make build

RPMTMP=$(mktemp -d 2>/dev/null || mktemp -d -t "rpmtmp")
trap 'rm -rf ${RPMTMP}' 0
"${REPO_DIR}/scripts/release/helper/build_rpm.sh" "${RPMTMP}"

I get the following build errors:

crypto/lib/libsodium.a(libsodium_la-utils.o): In function `explicit_bzero':
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:83: undefined reference to `__explicit_bzero_chk'
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:83: undefined reference to `__explicit_bzero_chk'
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:83: undefined reference to `__explicit_bzero_chk'
collect2: error: ld returned 1 exit status
make: *** [buildsrc] Error 2
build_release done packaging centos 20200108_033715

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, inside the rpm build inside the centos docker container I know we need the cleanup lines. The outer ubuntu build and the inner centos build are using the same checkout to ensure they have exactly the same source version. (We might be able to get rid of the entire 'centos build' now, we should do testing that current builds built on ubuntu18 run on ubuntu16 and centos7; this should work because of Tsachi's changes to make the build fully static)

The question I had about the build scripts was can we just skip the line make crypto/lib/libsodium.a that shows up in a few places.

Comment thread scripts/release/package.sh Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that you're missing set -e here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will set.

Comment thread scripts/release/socket.sh Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add set -e ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, in the case of this script you also want set the option so that and error on one pipe would break the entire command.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add the set -e. I'm afraid I didn't understand your second comment.

Comment thread scripts/release/Jenkinsfile Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's ok for now, but I would prefer to move this away from S3 and into github.
( like the reminder of our build sources ).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll either move this iteration or make a note to do so.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I built that compilation of gpg for centos. Centos7 ships with a version that is too old. I statically compiled a gpg that we can unpack into Centos7 and run. S3 is the right place for that archive.

Comment thread scripts/release/setup.sh Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any particular reason you're not using

GOPATH=$(go env GOPATH)
export GOPATH

?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember at one point that it couldn't find the go binary and so I manually set it to keep moving. I'll revisit this.

Copy link
Copy Markdown
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you move the files build.sh, package.sh and sign.sh, which are executed by the dynamically invoked host, to a sub-directory ? I'd like to create a separation between the "jenkins server" that is used to "orchestrate" the process and the actual running agent, which is building it.

@btoll btoll force-pushed the build_release branch 2 times, most recently from 7fb2f27 to 569170b Compare January 6, 2020 04:07
Copy link
Copy Markdown
Author

@btoll btoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you move the files build.sh, package.sh and sign.sh, which are executed by the dynamically invoked host, to a sub-directory ? I'd like to create a separation between the "jenkins server" that is used to "orchestrate" the process and the actual running agent, which is building it.

Yes, I'll do as you suggest.

@btoll btoll force-pushed the build_release branch 4 times, most recently from 89df002 to 08b685f Compare January 6, 2020 19:11
Benjamin Toll added 2 commits January 6, 2020 14:52
Most significantly, move the following files into a subdirectory to
emphasize a separation between Jenkins master controller and any build
agents:

- build.sh
- package.sh
- setup.sh
- sign.sh

Separated the `build and package` stage into its own parts.
Copy link
Copy Markdown
Contributor

@algobolson algobolson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should read the scripts in more detail. A couple early comments.

stages {
stage("create ec2 instance") {
steps {
sh script: 'scripts/release/start_ec2_instance.sh us-west-1 ami-0dd655843c87b6930 t2.2xlarge'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is that ami? comment how it was picked and when? Like, it's the "us-west-1 ubuntu 18.04 as of 2019-12-27" or so.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a comment.

Comment thread scripts/release/README.md

## TODO

Create the git tag.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disagree. I think tagging should remain a manual process. Then the build system picks that up. We decide "this is 2.1.42" and then the build system builds it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I have no problem with that.

}

/*
stage("tag") {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to have a "push button" tag, you can use an "input":
https://jenkins.io/doc/pipeline/steps/pipeline-input-step/

I think that we probably want to reverse this though, and only run the job for tagged commits.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That make sense. The workflow can be manually create a tag, then input that to Jenkins to pass to the build agent.

Yes, I'm already using the input step for both pausing the build and getting user input.

@algobolson
Copy link
Copy Markdown
Contributor

For each file that was copied scripts/* to scripts/release/... could you delete the original? git will hopefully detect that as a move and show the diff from the old version to the new version along with the move.

@btoll
Copy link
Copy Markdown
Author

btoll commented Jan 8, 2020

For each file that was copied scripts/* to scripts/release/... could you delete the original? git will hopefully detect that as a move and show the diff from the old version to the new version along with the move.

Yes, I wanted to keep them around just in case. Once everything looks good, I'll remove them.

@btoll btoll mentioned this pull request Jan 8, 2020
@btoll
Copy link
Copy Markdown
Author

btoll commented Jan 8, 2020

Closing in favor of #698

@btoll btoll closed this Jan 8, 2020
@btoll btoll deleted the build_release branch March 2, 2020 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants