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

build: inherit some flags from environment #1112

Closed
wants to merge 1 commit into from

Conversation

jbergstroem
Copy link
Member

OSTYPE and DESTCPU is in some cases (Jenkins, et al) provided as
environment variables. It's not enough to pass it to the Makefile
since gyp will generate incorrect information for building openssl
and other dependencies.

Found this while providing DESTCPU to a solaris environment and seeing openssl being built with -m64.

R=@rvagg

OSTYPE and DESTCPU is in some cases (Jenkins, et al) provided as
environment variables. It's not enough to pass it to the Makefile
since gyp will generate incorrect information for building openssl
and other dependencies.
@jbergstroem
Copy link
Member Author

@rvagg heads up, this might generate some fallout since our Jenkins currently use OSTYPE for it's own cause. We for instance use names like linux-gnu while --dest-os requires a set list. One alternative would be skipping this patch but having to provide patched values twice and the other would be rewriting everything so Makefile avoids picking environment up unless its provided by config.mk (which is generated by configure). Thoughts?

@bnoordhuis
Copy link
Member

Not a big fan of this patch. OSTYPE is a semi-standard environment variable on most Linux systems (and set to linux-gnu as you point out) so this change breaks configure without --dest-os. I think the fix should be sought in configuring Jenkins to pass the proper flags to configure, not by adding heuristics to the build scripts.

@jbergstroem
Copy link
Member Author

@bnoordhuis I think the bigger problem is that we're doing a mix of Makefile/configure logic. I don't care about OSTYPE as much as DESTCPU since gyp is pretty decent at picking that up. DESTCPU is required in our current setup if we want 32-bit builds on a 64-bit machine. Would it be ok to just fix that for now?

Down the road we'll most likely have better steps in jenkins/whatever build system we might be migrating in to as well.

@bnoordhuis
Copy link
Member

Couldn't this be solved by sticking a shell script in tools/ that runs exec $(dirname "$0")/../configure --dest-cpu=${DESTCPU} and making Jenkins run that instead of invoking configure directly?

@rvagg
Copy link
Member

rvagg commented Mar 10, 2015

I agree that $OSTYPE doesn't belong here, I simply co-opted that in Jenkins as the easiest platform detection mechanism I could find. I don't have a strong opinion on $DESTCPU though and can see the utility either way.

The current way we do this in Jenkins is set an ARCH environment variable in the Jenkins process which bubbles up to the Jenkins scripts so we can use it to pass back in to ./configure and a bunch of other things (e.g. we need it for release builds so we know what the output files should be called). I'm fine with continuing this way but the main drawback, which I think @jbergstroem is trying to get at, is that we are embedding a significant amount of logic in Jenkins that could live in either configure or Makefile. The Jenkins config is neither easy to extract nor to replicate (as far as I know so far), and this is a problem because it's only visible to the few of us who have full Jenkins access so we can't even accept contributions or have extra eyes critiquing our methods. nodejs/build#45 is an example of something that nobody outside of the core build team can even begin to help fix because of this problem.

Sorry for the :handwave: but that's all I have at the moment, no strong opinions.

@jbergstroem
Copy link
Member Author

I'm going to close this for now. What I'm not particularly keen about is that there seems to be this grey area between "build" and Jenkins.

I have a few ideas I'd like to test out (locally) to avoid patching similar to DESTCPU above which is where I'll start. I appreciate the feedback and discussion; it helped me realise that this wasn't the best way forward.

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

3 participants