-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
default variable ignored for openssl_fips
#2750
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Comments
More minimal example: {
'variables': {
'openssl_fips%': '',
'openssl_no_asm%': 0,
'conditions': [
['openssl_fips != ""', {
'openssl_product': '<(STATIC_LIB_PREFIX)openssl<(STATIC_LIB_SUFFIX)',
}, {
'openssl_product': '<(STATIC_LIB_PREFIX)openssl<(STATIC_LIB_SUFFIX)',
}],
],
},
}
{} Run
Then one gets
I'm still not sure what the proper fix is - it seems the condition evaluation happens before the variable default was set. |
My idea for a possible fix is to adjust LoadVariablesFromVariablesDict(variables, the_dict, the_dict_key) to happen before the recursion step into the --- /tmp/node-gyp/gyp/pylib/gyp/input.py 2022-10-19 12:15:28.240012241 +0200
+++ /tmp/portage/net-im/element-desktop-1.11.10/work/element-desktop-1.11.10/.hak/keytar/x86_64-unknown-linux-gnu/build/node_modules/node-gyp/gyp/pylib/gyp/input.py 2022-10-19 16:50:19.555358975 +0200
@@ -1304,6 +1334,8 @@
variables = variables_in.copy()
LoadAutomaticVariablesFromDict(variables, the_dict)
+ LoadVariablesFromVariablesDict(variables, the_dict, the_dict_key)
+
if "variables" in the_dict:
# Make sure all the local variables are added to the variables
# list before we process them so that you can reference one
@@ -1321,8 +1353,6 @@
the_dict["variables"], phase, variables, build_file, "variables"
)
- LoadVariablesFromVariablesDict(variables, the_dict, the_dict_key)
-
for key, value in the_dict.items():
# Skip "variables", which was already processed if present.
if key != "variables" and type(value) is str: |
From https://gyp.gsrc.io/docs/InputFormatReference.md#user_defined-variables:
I think this is because |
Python dicts are guaranteed to be insert order preserving in Python >= 3.7. |
There's some old discussion in https://groups.google.com/g/gyp-developer/c/1EWXAXe-qWs but maybe things could be different now in gyp-next with Python 3.7+ 🤷. |
The question is what should happen? If it should, then one needs to evaluate/set the default vars before descending into nested scopes (my proposed patch). If it should not, then electron has to somehow rewrite their |
FWIW I believe Electron 20 contains Node.js 16 (and this matches what is in the current https://github.com/nodejs/node/blob/v16.x/common.gypi). This is a redundant condition as both the branches assign the same value to |
Thanks! That should fix it, but seems like another workaround, possible here since the condition is redundant :) So in my opinion maybe the root cause should also be clarified/fixed, i.e. how to handle the default variable propagation. |
Opened nodejs/node#45076 for this specific case. Discussion around the general case is worth having, although I guess ultimately if changes were to be made they would go upstream in https://github.com/nodejs/gyp-next. |
Great, thanks! Out of curiosity, how could this ever have worked previously so that electron ships the file that way? |
I don't know about Electron but Node.js writes the
node-gyp uses that if custom headers are not used node-gyp/lib/create-config-gypi.js Lines 18 to 35 in d7687d5
|
I see, thanks - so the error in the
is that right? for my pracical issue - should we just wait until the fix is upstream in node-16 and electron ships the new gyp file, or do you see a better way? |
Could you check if
line? This file is written out by |
Ah. I think this may have already been fixed in node-gyp. At least I've found that node-gyp@9.0.0 works for me with a test addon:
whereas node-gyp@8.4.1 fails:
Looking at the changes that went into node-gyp@9.0.0 v8.4.1...v9.0.0 it looks like #2547 is the most likely to have fixed this as |
it does not define |
Any idea how we can properly resolve this issue? Thanks! Wait until electron depends on |
I've now "fixed" this "problem" for my concrete gentoo ebuild with a lot of duct-tape: SFTtech/gentoo-overlay@3eb701f I couldn't just depend on node-16 because some dependency |
Both paths for the condition being removed result in the same value being assigned to `openssl_product`. This condition was also problematic as it was testing a variable in the same scope which gyp/gyp-next currently does not support. Refs: https://gyp.gsrc.io/docs/InputFormatReference.md#user_defined-variables PR-URL: nodejs#45076 Refs: nodejs/node-gyp#2750 Refs: nodejs#38633 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
Both paths for the condition being removed result in the same value being assigned to `openssl_product`. This condition was also problematic as it was testing a variable in the same scope which gyp/gyp-next currently does not support. Refs: https://gyp.gsrc.io/docs/InputFormatReference.md#user_defined-variables PR-URL: nodejs/node#45076 Refs: nodejs/node-gyp#2750 Refs: nodejs/node#38633 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
Both paths for the condition being removed result in the same value being assigned to `openssl_product`. This condition was also problematic as it was testing a variable in the same scope which gyp/gyp-next currently does not support. Refs: https://gyp.gsrc.io/docs/InputFormatReference.md#user_defined-variables PR-URL: nodejs/node#45076 Refs: nodejs/node-gyp#2750 Refs: nodejs/node#38633 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
Without this workaround node18 fails to generate prebuilds for node versions < 18 with: gyp: name 'openssl_fips' is not defined while evaluating condition 'openssl_fips != ""' in binding.gyp while trying to load binding.gyp
I think this has now been fixed in newer electron releases. |
I can't build element-desktop with node18 due to the error message:
openssl_fips
used in/tmp/portage/net-im/element-desktop-1.11.10/homedir/.electron-gyp/20.1.4/include/node/common.gypi
,which is fetched from https://electronjs.org/headers/v20.1.4/node-v20.1.4-headers.tar.gz and put in
npm_config_devdir
, i.e.~/.electron-gyp
.include/node/common.gypi
looks like this:The definition of
openssl_fips% = ''
is also in the file, so according to https://gyp.gsrc.io/docs/InputFormatReference.md variables ending with % should be used as default value if not set previously.But that mechanism doesn't seem to kick in:
node-gyp/gyp/pylib/gyp/__init__.py
sodef main(args):
only doesreturn gyp_main(args)
without swallowing the very useful backtrace.node_modules/node-gyp/gyp/pylib/gyp/input.py
'sEvalSingleCondition
just before the crash'openssl_fips%': ''
, butopenssl_fips
is unset. The condition expression from the node-headers then can't be evaluated since the variable is not available whenEvalSingleCondition
runseval(ast_code, ..., variables)
.-> either, the condition has to lookup the variable with a
%
appended, or it needs to be in the variables dict without a%
. I guess the latter should be the case.I haven't found yet where exactly the bug is, or if it even is a bug in node-gyp. But maybe you already have some idea :)
Verbose output (from npm or node-gyp):
The text was updated successfully, but these errors were encountered: