Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a drake binary in CI instead of compiling it from source #401

Merged
merged 3 commits into from
May 11, 2018

Conversation

apojomovsky
Copy link

@apojomovsky apojomovsky commented May 9, 2018

Fixes #352 , which is also reflected on the 5th element of the list on #378 .

  • Avoids the compilation step we used to have for drake in CI and instead, downloads a binary correspondent to the hash we are pointing in the delphyne.repos file.
    • This URL has to be manually updated every time we update that hash.
  • Adds wget as a dependency.

Obs: cloning the drake repository is still required here, because although it's not built from source anymore, it's still required to run the install_prereqs.sh script to solve drake's runtime dependencies.

With this upgrade the time required for the build step goes down from 16:26 minutes to 2:31 minutes :)
screenshot from 2018-05-09 17-04-45
screenshot from 2018-05-09 17-04-18

@basicNew
Copy link

basicNew commented May 9, 2018

This is great, thanks @apojomovsky !

Couple of thoughts:

  • Can't we relate the hash in delphyne.repos with the date it was merged and hence derive the build timestamp? That way we don't have to do the dual-update dance.
  • While unlikely, it may be the case that another (breaking) commit gets merged that same day after the one we are pointing to, and hence the build breaks, even if it locally builds ok. I think that the odds are small enough not to block on that, but would like to hear other opinions. @stonier @clalancette @caguero thoughts?

@clalancette
Copy link
Contributor

Couple of thoughts:

  • Can't we relate the hash in delphyne.repos with the date it was merged and hence derive the build timestamp? That way we don't have to do the dual-update dance.

I agree that we should try to avoid hard-coding this here, as we'll probably forget to do it later on. It would be best to do what you say, and derive the date from the commit hash. This may be easier said than done, though :).

  • While unlikely, it may be the case that another (breaking) commit gets merged that same day after the one we are pointing to, and hence the build breaks, even if it locally builds ok. I think that the odds are small enough not to block on that, but would like to hear other opinions. @stonier @clalancette @caguero thoughts?

Yeah, I see what you mean. While that would be a bit unfortunate, I agree that it is not really a huge problem; we'll notice the next day. I don't think we need to worry too much about that, so I'd say forge ahead despite that.

@stonier
Copy link
Collaborator

stonier commented May 9, 2018

Can't we relate the hash in delphyne.repos with the date it was merged and hence derive the build timestamp? That way we don't have to do the dual-update dance.

There's always the chance that the nightly build doesn't exist too (can happen if it fails to build that day).

Another option is to have a jenkins job of our own that is responsible for taking the hash in our delphyne.repos file and building our own tarball nightly. Even better if it can track that hash and only do so if the hash changes. You'd have to talk to Jamie to work out how to do this if we do it on Drake's Jenkins and whatever server is file serving the tarballs.

@clalancette
Copy link
Contributor

Oh, there's one more thing I want to check. Are those drake tarballs the output from clang or gcc? I think I remember @stonier telling me that they were all gcc, but I just want to make sure, since our stuff definitely won't work with clang.

@stonier
Copy link
Collaborator

stonier commented May 9, 2018

Are those drake tarballs the output from clang or gcc?

GCC

@apojomovsky apojomovsky force-pushed the apojomovsky/use_drake_binary_in_ci branch from 6a80db7 to eef0780 Compare May 10, 2018 15:04
@apojomovsky
Copy link
Author

apojomovsky commented May 10, 2018

Another option is to have a jenkins job of our own that is responsible for taking the hash in our delphyne.repos file and building our own tarball nightly.

I'd really like this, although I'd put it on a separate ticket and tackle it on a following iteration. Would you like me to create that ticket @stonier ?

@apojomovsky apojomovsky force-pushed the apojomovsky/use_drake_binary_in_ci branch from eef0780 to 1d25131 Compare May 10, 2018 15:14
@apojomovsky
Copy link
Author

Just updated the PR with the ability to parse the commit date automatically and use it to generate the binary's URL.
Now working on the logic to check if the nightly build exists and if not, get the one from the day after it.

@apojomovsky apojomovsky force-pushed the apojomovsky/use_drake_binary_in_ci branch from 9111d73 to 379c3d3 Compare May 10, 2018 19:21
@apojomovsky
Copy link
Author

apojomovsky commented May 10, 2018

I've just updated the PR with the logic to check if the nightly build URL corresponds to a valid binary.
So in case it doesn't exists, the script will attempt to download the binary from the day after the commit date. This will run for up to 5 different dates (this number was arbitrary chosen) and if none of them are valid, will fail with a message. Any feedback is welcome @basicNew , @clalancette , @stonier

Copy link

@basicNew basicNew left a comment

Choose a reason for hiding this comment

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

Great! Super minor: would be great if you can avoid duplicating the BINARY_URL harcoded value and have a pattern and a replacement. But again, not a deal breaker at all.

@apojomovsky apojomovsky force-pushed the apojomovsky/use_drake_binary_in_ci branch from 379c3d3 to 944d7ff Compare May 10, 2018 20:05
@apojomovsky apojomovsky force-pushed the apojomovsky/use_drake_binary_in_ci branch from 944d7ff to a167a94 Compare May 10, 2018 20:18
@apojomovsky
Copy link
Author

Thanks @basicNew ! Just did that.

@basicNew
Copy link

Unless someone is against merging this I would say gogogo! @clalancette @stonier ?

@stonier
Copy link
Collaborator

stonier commented May 11, 2018

No problems here, can you make sure we have instructions in the developers section of the delphyne guide somewhere so we have a 'process' recorded somewhere for updating drake?

Also future proof that part too - record our thoughts that it's not ideal right now, what the expected problems are (mismatching or missing binary), and what would be better iterations (a separate job that uploads our binary alongside the other drake binaries, moving internally to our own jenkins/file server).

@clalancette
Copy link
Contributor

This all seems reasonable enough to me now (with the documentation that @stonier suggested), so let's merge and see what happens.

@apojomovsky
Copy link
Author

Thanks for the feedback @clalancette and @stonier .
I just created an issue for having a separate CI job that builds our own drake tarball nightly: #406 .
I've also added a description for the drake update process to the delphyne guide here .
Considering that I already have your green light here, I'll proceed to merge this issue now.

@apojomovsky apojomovsky merged commit 3617cc2 into master May 11, 2018
@apojomovsky apojomovsky deleted the apojomovsky/use_drake_binary_in_ci branch May 11, 2018 19:14
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