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

Feature/issue 1213 Trimming boost #1223

Merged
merged 11 commits into from May 13, 2019

Conversation

Projects
None yet
3 participants
@rok-cesnovar
Copy link
Collaborator

commented May 3, 2019

Summary

The main objective of this PR is to trim the Boost library folder in /lib. With this PR we get from 66828 items, totalling 608,4 MB to 22222 items, totalling 193,9 MB on the Boost folder.

This PR also adds the upgrade-boost.sh script that can be used to upgrade to newer versions. In this case it was used to "upgrade" from 1.69 to 1.69 and do the trimming/pruning.

It also edits the upgrade-sundials.sh with an added -u flag to git add make/*. Without the flag this throws an error if the developer has a make/local file (which should be ignored but we try to add it).

Tests

/

Side Effects

Hopefully none.

Checklist

  • Math issue #1213

  • Copyright holder: Rok Češnovar

  • the basic tests are passing

  • the code is written in idiomatic C++ and changes are documented in the doxygen

  • the new changes are tested

@rok-cesnovar rok-cesnovar requested a review from syclik May 3, 2019

@rok-cesnovar

This comment has been minimized.

Copy link
Collaborator Author

commented May 4, 2019

I manually commented out the BOOST_HEADER_DEPRECATED in pending/integer/integer_log2.hpp. I decided not to include it in the script since it was fixed for 1.70.0. See discussion here and this commit. The next upgrade of Boost is going to be to 1.70.0+ so I feel that it does not need to be automated. I could also upgrade do 1.70.0, but since we have no other incentive to do this, I feel that is not necessary.

@syclik

This comment has been minimized.

Copy link
Member

commented May 8, 2019

I'll take a look soon. Any guidance on what I should look for? Do you know if the build failure is real?

@rok-cesnovar

This comment has been minimized.

Copy link
Collaborator Author

commented May 8, 2019

I dont think there is a better way than to go through the 5 commits that are actually readable/reviewable. The other 3 commits are done by the script so they can be reviewed that way.

The commits and the description of the changes:

03a88d7 adds the -u flag to the upgrade sundials script (explained above in the PR post).

14312e2 this one adds the upgrade boost file and will be the largest change to review. Its mostly a copy of the sundials one with the cleanup at the end.

c391f42 chmod changes on the upgrade-boost.sh

7d37e8b
just adds a check to the script file. If the versions are the same, we dont need to modify the numbers in the makefiles and README. Why even support "upgrading" to the same version? If someone finds some new ways of trimming Boost, they can add the lines in the pruning phase of the script and run it. If you feel this isnt necessary, I can remove it.

b5f502a I went and manually commented out the deprecated header warning in the boost/random. In 1.0.70 this is no longer needed so I decided not to include this in the script but rather add it manually.

Regarding the testing failure:
I am experiencing the same issue that Sebastian fixed in #1082 (comment) I havent figured out why the tools/build subdirectory is not included when using the script. I got busy with other stuff so havent gotten to this yet (I stopped the last Jenkins run on this branch, to no clobber the system cause I knew it would fail) . You can dismiss the review request if you want and I will re-request later when I fix this.

@syclik

This comment has been minimized.

Copy link
Member

commented May 9, 2019

03a88d7 adds the -u flag to the upgrade sundials script (explained above in the PR post).

It also edits the upgrade-sundials.sh with an added -u flag to git add make/*. Without the flag this throws an error if the developer has a make/local file (which should be ignored but we try to add it).

Awesome. I didn't even realize that would be a problem.

14312e2 this one adds the upgrade boost file and will be the largest change to review. Its mostly a copy of the sundials one with the cleanup at the end.

Great. I just looked through that and it looks clean enough for someone else to pick up.

7d37e8b
just adds a check to the script file. If the versions are the same, we dont need to modify the numbers in the makefiles and README. Why even support "upgrading" to the same version? If someone finds some new ways of trimming Boost, they can add the lines in the pruning phase of the script and run it. If you feel this isnt necessary, I can remove it.

That's great! I think that's a perfectly good use case.

b5f502a I went and manually commented out the deprecated header warning in the boost/random. In 1.0.70 this is no longer needed so I decided not to include this in the script but rather add it manually.

What's happening here? Do we have a lot of warnings? Do we include the deprecated header directly? Is this a current problem on develop?

@rok-cesnovar

This comment has been minimized.

Copy link
Collaborator Author

commented May 9, 2019

What's happening here? Do we have a lot of warnings? Do we include the deprecated header directly? Is this a current problem on develop?

We dont have any warnings on develop because this was already commented out on the PR that upgraded to 1.69.0 (see last commit in #1082). Its not included directly but through including boost/random and has been fixed for 1.70.0 (boostorg/random#49).

@rok-cesnovar

This comment has been minimized.

Copy link
Collaborator Author

commented May 9, 2019

So the issue is that lib/boost_*/tools/build/ is in .gitignore, but the tests need these files:

cd lib/boost_1.69.0; ./bootstrap.sh
./bootstrap.sh: 1: ./bootstrap.sh: ./tools/build/src/engine/build.sh: not found
Building Boost.Build engine with toolset ... 
Failed to build Boost.Build build engine

I am not sure how, but @wds15 got this directory through in the last Boost upgrade, but you can see that they were facing the same issue in that PR #1082 (comment)

I am also tagging @seantalts since he added those lines to .gitignore (4fbb427#diff-a084b794bc0759e7a6b77810e01874f2)

@seantalts

This comment has been minimized.

Copy link
Member

commented May 9, 2019

@rok-cesnovar

This comment has been minimized.

Copy link
Collaborator Author

commented May 10, 2019

I removed the tools/build/src from .gitignore dfae418 and added the files that are no longer ignored. Boost now builds.

@rok-cesnovar

This comment has been minimized.

Copy link
Collaborator Author

commented May 11, 2019

@syclik this in now ready for another review. See comment above for a short description of changes.

@syclik

syclik approved these changes May 13, 2019

Copy link
Member

left a comment

This looks sane to me. Thanks!

@syclik syclik merged commit aa1d710 into develop May 13, 2019

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details

@syclik syclik deleted the feature/issue-1213-trimming-boost branch May 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.