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

Update DevKit build "bootstrap" to ensure all tooling comes from bootstrap #1043

Merged
merged 3 commits into from
Jun 19, 2024

Conversation

andrew-m-leonard
Copy link
Contributor

All the necessary DevKit tooling (ld, as, ar, ranlib,...) needs explicitly setting to the bootstrap to ensure the final DevKit is bootstrapped correctly.

Copy link

Thank you for creating a pull request!

Please check out the information below if you have not made a pull request here before (or if you need a reminder how things work).

Code Quality and Contributing Guidelines

If you have not done so already, please familiarise yourself with our Contributing Guidelines and Code Of Conduct, even if you have contributed before.

Tests

Github actions will run a set of jobs against your PR that will lint and unit test your changes. Keep an eye out for the results from these on the latest commit you submitted. For more information, please see our testing documentation.

In order to run the advanced pipeline tests (executing a set of mock pipelines), it requires an admin to post run tests on this PR.
If you are not an admin, please ask for one's attention in #infrastructure on Slack or ping one here.
To run full set of tests, use "run tests"; a subset of tests on specific jdk version, use "run tests quick 11,21"

@andrew-m-leonard
Copy link
Contributor Author

@sxa for review please

Copy link
Member

@sxa sxa left a comment

Choose a reason for hiding this comment

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

@andrew-m-leonard Is this intended to resolve the glibc dependency issue while building the devkit? I tried it on F38 and it seems to hit the same issue.

@andrew-m-leonard
Copy link
Contributor Author

andrew-m-leonard commented Jun 17, 2024

@andrew-m-leonard Is this inteded to resolve the glibc dependency issue while building the devkit? I tried it on F38 and it seems to hit the same issue.

@sxa No that's a different (altough possibly related) problem, this is purely to resolve reliable independent DevKit building, ie.ensuring all the gcc dependencies are bootstrapped in building gcc, so that no different OS dependency can cause a difference.
Ie.all of CC, CXX, LD, AR, AS, RANLIB and OBJDUMP, come from the bootstrap build.

Copy link
Member

@sxa sxa left a comment

Choose a reason for hiding this comment

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

I think this is ok but to be clear what was the reason for changing from the environment variables for CC/CXX to putting them on the make command line? Was that significant? Is there any potential problem if, say, the compiler calls ld internally that might result in it not using the one from the devkit in this situation?
It may be nothing, but just a though when I saw the changing in where it is defined.

@andrew-m-leonard
Copy link
Contributor Author

andrew-m-leonard commented Jun 18, 2024

I think this is ok but to be clear what was the reason for changing from the environment variables for CC/CXX to putting them on the make command line? Was that significant? Is there any potential problem if, say, the compiler calls ld internally that might result in it not using the one from the devkit in this situation? It may be nothing, but just a though when I saw the changing in where it is defined.

There's no fully guarding against what each sub-dependency checks to find things in their Makefiles, but being consistent with what I have seen and tested, by adding make variables and ensuring PATH contains the bootstrap bin, seems to cover everything.

@andrew-m-leonard
Copy link
Contributor Author

I think this is ok but to be clear what was the reason for changing from the environment variables for CC/CXX to putting them on the make command line? Was that significant? Is there any potential problem if, say, the compiler calls ld internally that might result in it not using the one from the devkit in this situation? It may be nothing, but just a though when I saw the changing in where it is defined.

@sxa Which do you think is better, export or make variable ? or should we just do both...?

@sxa
Copy link
Member

sxa commented Jun 18, 2024

I think this is ok but to be clear what was the reason for changing from the environment variables for CC/CXX to putting them on the make command line? Was that significant? Is there any potential problem if, say, the compiler calls ld internally that might result in it not using the one from the devkit in this situation? It may be nothing, but just a though when I saw the changing in where it is defined.

@sxa Which do you think is better, export or make variable ? or should we just do both...?

Honestly I'm pretty confident that once you've set the PATH you're fine if the tools aren't suffixed (i.e. they don't have ld-2.39 instead of just ld). I've generally used the environment variables rather than parameters to make (which is why I asked the question originally rather than me having deep insight here!) but that's what you had originally (and I guess you saw some issues with it?) From a quick test it seems to manage to find them in the PATH even if CC etc. are set to gibberish values ...

@andrew-m-leonard
Copy link
Contributor Author

I think this is ok but to be clear what was the reason for changing from the environment variables for CC/CXX to putting them on the make command line? Was that significant? Is there any potential problem if, say, the compiler calls ld internally that might result in it not using the one from the devkit in this situation? It may be nothing, but just a though when I saw the changing in where it is defined.

@sxa Which do you think is better, export or make variable ? or should we just do both...?

Honestly I'm pretty confident that once you've set the PATH you're fine if the tools aren't suffixed (i.e. they don't have ld-2.39 instead of just ld). I've generally used the environment variables rather than parameters to make (which is why I asked the question originally rather than me having deep insight here!) but that's what you had originally (and I guess you saw some issues with it?) From a quick test it seems to manage to find them in the PATH even if CC etc. are set to gibberish values ...

The issues we had were from a 3rd party (in this case Red Hat) independent DevKit build, which was different, and although we've tracked down the exact difference, I want to try and do "belt & braces" on all the variables, i'm thinking maybe changing to PATH and export... what do you think?

Copy link
Member

@sxa sxa left a comment

Choose a reason for hiding this comment

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

The issues we had were from a 3rd party (in this case Red Hat) independent DevKit build, which was different, and although we've tracked down the exact difference, I want to try and do "belt & braces" on all the variables, i'm thinking maybe changing to PATH and export... what do you think?

If the "exact difference" referenced above was unrelated to these variables I'd personally prefer not to set them unless there was a known issue with the previous PATH setting not doing what you expect. I'd be nervous that adding in potentially unnecessary definitions into this (and increasing code complexity) may lead people to a false sense that changing them in the future might do "something".

Having said that, I don't want to block on this, so I can approve regardless as I don't think it'll actively damage anything.

@andrew-m-leonard
Copy link
Contributor Author

The issues we had were from a 3rd party (in this case Red Hat) independent DevKit build, which was different, and although we've tracked down the exact difference, I want to try and do "belt & braces" on all the variables, i'm thinking maybe changing to PATH and export... what do you think?

If the "exact difference" referenced above was unrelated to these variables I'd personally prefer not to set them unless there was a known issue with the previous PATH setting not doing what you expect. I'd be nervous that adding in potentially unnecessary definitions into this (and increasing code complexity) may lead people to a false sense that changing them in the future might do "something".

Having said that, I don't want to block on this, so I can approve regardless as I don't think it'll actively damage anything.

I'll leave it as it is now, as we have independently verified 3rd party DevKit reproducible with the above settings.
thanks

@andrew-m-leonard andrew-m-leonard merged commit 03479cd into adoptium:master Jun 19, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants