-
Notifications
You must be signed in to change notification settings - Fork 168
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
ansible: update minimum gcc on linux-ppc64le to gcc-6 #1723
Conversation
Actually let's wait with this as little bit. I want to test GCC-8 binaries on machines with only 4.9. |
Pushed a commit. When testing on a new PPC machine the dependencies were not quite right between roles. The new commit fixes that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Created a new machine (test-osuosl-ubuntu1404-ppc64_le-4), configure it with this script and test job to ensure builds ok without changes to select-compiler. |
@refack in terms of testing GCC 8 binaries on machine with only 4.9.4 one thing to keep in mind is that it depends on what symbols are used from the libraries that come with the compiler. Even it things run today (I think we've seen problems in the past) there is nothing stopping the use of new of new symbols in the future which might break the backwards compatibility. The great thing about the RHEL developer toolset is that it understands the deltas introduced in newer gcc versions and links statically instead of dynamically (based on my understanding) avoiding that problem. From the IBM perspective I understand that our binaries might not be quite as fast having been built with an older gcc, but I value the stability/compatibility of using the older compiler more until we can get the developer toolset in place. |
A minimal sanity test is checking that the libraries have a minimal version. The error we're seeing now is:
while the library does export it's minimal supported version:
|
Another simple option is to simply statically link P.S. that's who it's done for Windows |
@refack back when we ramped up support for PPC we explored the available options including statically linking. Unless we do that for all platforms (x86) included we did not feel it was a good option. I think the developer toolset has progressed and has better platform support (as does our options for OS's at OSU) but as mentioned I don't think we can count on figuring that out before 12.X goes out. |
@sam-github noticed one other problem in that after setup on a new machine the wrong gcc is the default. We need to figure out how to update the ansible script to fix that |
I think the select compiler script will still do the right thing if the default is wrong, but it would be better if the default was set properly for cases were we have not properly configured the jobs. |
@rvagg noticed that too in #1722 (comment) I think this will require us to use a compiler selector for all platforms. (I'm now thinking that we should start storing that information in the node repo and read it for each checkout) |
Build with PR being used to select the compiler as gcc-6 : https://ci.nodejs.org/job/node-test-commit-plinux-mdawson-test/nodes=test-osuosl-ubuntu1404-ppc64_le-4/2/ |
re: #1723 (comment) I don't see why the default matters, we explicitly set the compiler version with select-compiler.sh. If someone forgets to use select-compiler.sh, they will get the wrong compiler for some cases (it will be wrong for 9.x+, or wrong for 11.x+, but can't be correct for both). I've no idea how to change the default. I'll check, but I suspect it depends on the order in which packages are installed, which depends on current machine state, so is indeterminant. |
@@ -106,6 +106,6 @@ packages: { | |||
], | |||
|
|||
ubuntu1404: [ | |||
'ntp,g++-4.8,gcc-4.8,g++-4.9,gcc-4.9,binutils-2.26', | |||
'ntp,gcc-6,g++-6,g++-4.8,gcc-4.8,g++-4.9,gcc-4.9,binutils-2.26', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switching order might make the last installed the default, but if gcc-4.9 is already installed, YMMV
https://askubuntu.com/questions/26498/how-to-choose-the-default-gcc-and-g-version But you are right it doesn't matter. |
P.S. maybe we should run:
So there will be no default... But that's for a separate PR. |
yeah, I like that a lot: always explicit and fail when unspecified |
My ansible skills are barely up to copy-n-paste, they are not up to adding a new shell command like #1723 (comment). I don't know where to put it, and I don't know how to make sure it only runs after the packages are installed, and ideally, run after if-and-only-if packages are installed. I can test it if I'm given some pointers to what kind of config to put in what place. |
@sam-github IMHO this PR looks good as is. The default discussion just gravitated here. I'll pick it up in a separate PR. |
@mhdawson PTAL, to make sure the changes you pushed are back |
@sam-github I think it looks good. |
So given that we agree this can move forward we need to FIRST configure all of the machines to have gcc6 otherwise it will break test/build. |
Perhaps we could we break this up into two parts:
|
Breaking this into two commits is trivial, if that's the right way, I can do it. Note that @mhdawson is already in process of using the PR branch to update the machines, so the whole PR can be merged. |
Another off-shoot idea. Store P.S. I'm working on it (as I see it's very natural to add that logic to |
I'm fine either way (1 PR or 2). 2 is probably better in case we need to revert the compiler selection for some reason. in test le-2, le-4 are updated I'll plan to do le-1 later today and therelease machine some time next week (or possibly the week after since I'm on holiday next week even though I'm not going anywhere.) |
Deployed across the 3 test machines and the release machine. Should be good to land. After landing please what both test/release jobs for a few days to make sure things look ok. One extra todo is that we need to add ubuntu1404-ppc64_le-4: {ip: 140.211.168.140} to inventory.yml. I created that as a replacement for ubuntu1404-ppc64_le-3 which was completely broken |
Landed as 03631de |
See: #1723 (comment) PR-URL: #1739 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
No description provided.