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

configure: Delete config_fips.gypi when not needed #4632

Closed
wants to merge 1 commit into from

Conversation

stefanmb
Copy link
Contributor

If reconfiguring without fips, do not leave stale config_fips.gypi on disk. It will be recreated if needed by re-running configure.

If reconfiguring without fips, do not leave config_fips.gypi on disk.
It will be recreated if needed by re-running configure.
@silverwind silverwind added the build Issues and PRs related to build files or the CI. label Jan 11, 2016
@rvagg
Copy link
Member

rvagg commented Jan 12, 2016

make distclean takes care of this, I don't think this is a job for configure

@stefanmb
Copy link
Contributor Author

@rvagg That's true, but if you run configure twice without runing distclean in between the FIPS configuration file will be left over.

This changeset resulted from an attempt at auto-detecting FIPS mode in relation to #4631. I eventually gave up on the auto-detection because I felt it complicated the Makefile too much, but in the process I noticed the config_fips.gypi file would be leftover on disk and thus was not a reliable way to detect if we are indeed compiling in FIPS mode.

However, if you think the functionality is unnecessary I can close this request.

@rvagg
Copy link
Member

rvagg commented Jan 12, 2016

I'd like to hear other voices from @nodejs/build and @nodejs/crypto on this, I hear you on the complication but doing a clean from configure feels very wrong.

@jbergstroem
Copy link
Member

-1 from deleting in configure but also agree that we're not catering to the problem at hand.

@jasnell
Copy link
Member

jasnell commented Jan 12, 2016

hmm... I think I agree with @rvagg on this. If fips mode is not enabled in the second run of configure then I would simply expect the existing file to be ignored, not deleted. It should only be deleted when doing a clean

@stefanmb
Copy link
Contributor Author

@jasnell @jbergstroem @rvagg

Sounds good, I understand the rationale and agree with it. I'll close this pull request. Thanks!

@stefanmb stefanmb closed this Jan 12, 2016
@rvagg
Copy link
Member

rvagg commented Jan 13, 2016

can someone explain the rationale for having config_fips.gypi? the comment in there suggests its all about node-gyp, is that the only reason we have a second file?

@stefanmb
Copy link
Contributor Author

@rvagg I introduced that file. The rationale was explained in #4023. The file is used to separate out the special linking directive needed for FIPS from that used by node-gyp, so that modules built using your FIPS-enabled Node.js installation don't also require the OpenSSL source to be present. However, I'm open to suggestions of better, low impact ways of achieving the same thing.

@rvagg
Copy link
Member

rvagg commented Jan 13, 2016

I wish I did have a better suggestion, but this is gyp so ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants