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

Fix versioned GCC detection #552

Closed
wants to merge 2 commits into from
Closed

Fix versioned GCC detection #552

wants to merge 2 commits into from

Conversation

ghedo
Copy link
Contributor

@ghedo ghedo commented Jan 14, 2016

Basically this fixes "make depend" when using e.g. "CC=gcc-5" and also makes Travis run "make depend".

When using versioned GCC executables (e.g. gcc-5, gcc-6, ...) running
"make depend" fails because $ecc is compared to the "gcc" string (without
version) when setting the MAKEDEPPROG variable.

Also, since now $ecc will be just "gcc", simplify the --strict-warnings
check.
@ghedo
Copy link
Contributor Author

ghedo commented Jan 14, 2016

@ekasper seems that your commit 58dd1ce broke this, so you might want to take a look to see if my fix is ok.

@levitte I'm not sure if running make depend on Travis is the right thing, care to comment?

@levitte
Copy link
Member

levitte commented Jan 14, 2016

Running make depend is only strictly necessary if you do repeated updates and builds in the same directory. Since travis builds always starts with a clean dir, a make depend isn't necessary there.

@ghedo
Copy link
Contributor Author

ghedo commented Jan 14, 2016

@levitte yes, but with the recent build changes we always get the "you must run make depend" message even on Travis: https://travis-ci.org/openssl/openssl/jobs/102411115#L6185

Maybe it's a separate bug...

@ghedo
Copy link
Contributor Author

ghedo commented Jan 14, 2016

Also, IIRC @richsalz mentioned somewhere that running "make depend" is mandatory now (or did I just imagine it?), maybe I should have asked him instead :)

@levitte
Copy link
Member

levitte commented Jan 14, 2016

We are a bit over the top there, it's true... I did see it as mandatory just a few days ago, but then realised that for those who just download, unpack, build, test and install and then don't touch it any more, that's strictly speaking not true.

... I'm way too sleepy to do anything about it now, but if noone gets there before me, I might think of some way to reword that "error" message tomorrow...

@levitte
Copy link
Member

levitte commented Jan 14, 2016

What really is mandatory is to run config or Configure... I've come to understand that there were some who skipped that step (don't ask me how).

@ghedo
Copy link
Contributor Author

ghedo commented Jan 14, 2016

Ok, thanks. We might still want to run it on Travis for testing purposes though (e.g. doing that wold have caught the error I got when running make depend with CC=gcc-6 much sooner).

@ekasper
Copy link
Contributor

ekasper commented Jan 18, 2016

@ghedo - I can't quite follow how my commit broke it; it would seem that makedepend detection was already broken before that, because it's testing for equality with "gcc". Anyway, fix seems reasonable, @levitte is the best person to approve the MR.

@ghedo
Copy link
Contributor Author

ghedo commented Jan 18, 2016

@ekasper hmmm, you are right, I don't remember my original thought process, I guess I was just too quick to jump to conclusions, sorry :/

@ghedo
Copy link
Contributor Author

ghedo commented Jan 19, 2016

@levitte ping? should I just remove the "make depend on Travis" commit?

@richsalz
Copy link
Contributor

wait a bit for my "remove clean-depend" change to land, and let's see if this is still needed.

@ghedo
Copy link
Contributor Author

ghedo commented Feb 1, 2016

This is fixed in master now.

@ghedo ghedo closed this Feb 1, 2016
@ghedo ghedo deleted the fix_gcc branch February 2, 2016 20:22
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.

None yet

4 participants