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.

Sign up for GitHub

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

Jump to bottom

lto build failed with ninja #40509

Closed
gengjiawen opened this issue Oct 19, 2021 · 7 comments
Closed

lto build failed with ninja #40509

gengjiawen opened this issue Oct 19, 2021 · 7 comments
Assignees
@danbev
Labels
build Issues and PRs related to build files or the CI.

Comments

@gengjiawen
Copy link
Member 

ninja: Entering directory `out/Release'
ninja: error: obj/deps/openssl/openssl-fipsmodule.ninja:879: bad $-escape (literal $ must be written as $$)
    -Wl,--version-script=$(srcdir)/deps/openssl/config/archs/linux-x86_6...
                         ^ near here

cc @danbev @nodejs/build-files

https://github.com/nodejs/node/runs/3933310474?check_suite_focus=true
@Mesteery Mesteery added the build Issues and PRs related to build files or the CI. label Oct 19, 2021
@danbev
Copy link
Contributor

danbev commented Oct 19, 2021 •
edited
Loading

This is caused by a variable that is being used for the version/linker script which was a late fix as it was an absolut path previously.

To reproduce:

$ configure --openssl-is-fips --ninja
$ ./ninja -C out/Release

One option might be to include config.mk in deps/openssl/config/Makefile and then pass BUILD_WITH to deps/openssl/config/generate_gypi.ln where it could check the type of build that is configured. But since these generated files are then checked in that would not work for make then and only ninja and we would have to perhaps have an extra openssl-fips.gypi-template for ninja build which does not sounds very nice to me.

Just trying this out locally there is another issue with the FIPS build and ninja after fixing the escape character above:

$ ninja -v -C out/Release
ninja: Entering directory `out/Release'
ninja: error: 'obj/deps/openssl/libopenssl-fipsmodule.so', needed by 'obj/deps/openssl/fipsmodule.cnf', missing and no known rule to make it

Perhaps we could use make for this job or alternatively remove the --openssl-is-fips flag until this issue is fixed?
I've got a suggestion for fixing this and I'll link to a PR shortly.

@danbev danbev self-assigned this Oct 19, 2021
@richardlau
Copy link
Member

richardlau commented Oct 19, 2021 •
edited
Loading

@danbev I'm a bit confused -- the daily LTO GitHub actions builds don't use the --openssl-is-fips configure flag:

node/.github/workflows/daily.yml

Line 27 in f69a4f6

./configure --enable-lto --ninja

@danbev
Copy link
Contributor

danbev commented Oct 19, 2021

Hmm, I was able to reproduce this using --enable-fips but perhaps this also happens with --enable-lto in combination with --ninja. I'll give that a try as well but I think I've got something that will work for both make and ninja.

danbev added a commit to danbev/node that referenced this issue Oct 19, 2021
@danbev
deps,build,tools: fix openssl-is-fips for ninja builds
757e147 
Currently using the --openssl-is-fips configuration option in
combination with --ninja is broken.

This commit fixes two issues, one being an issue with the linker/version
script path variable. The second is that the locations of built
artifacts that differ for ninja and make.

ninja:
$ ./configure --openssl-is-fips --ninja
$ ninja -C out/Release
$ ./node --enable-fips -p 'crypto.getFips()'
1

make:
$ ./configure --openssl-is-fips
$ make -j8
$ ./node --enable-fips -p 'crypto.getFips()'
1

Refs: nodejs#40509
danbev added a commit to danbev/node that referenced this issue Oct 19, 2021
@danbev
deps: regenerate OpenSSL arch files
7cf53e5 
Refs: nodejs#40509
@danbev danbev mentioned this issue Oct 19, 2021
Closed
@danbev
Copy link
Contributor

danbev commented Oct 19, 2021

Does anyone know of a way to re-run that job but using the the linked PR?

@targos
Copy link
Member

targos commented Oct 19, 2021 •
edited
Loading

Does anyone know of a way to re-run that job but using the the linked PR?

AFAIK, you can only do that if the job is configured for it with inputs (like this one in CITGM)

@danbev
Copy link
Contributor

danbev commented Oct 19, 2021 •
edited
Loading

I was able to reproduce this on master but not on the branch used in the linked pr. So hopefully this will take care of this issue. I'll let the build complete and verify that is succeeds, and it did ✔️

danbev added a commit that referenced this issue Oct 21, 2021
@danbev
deps,build,tools: fix openssl-is-fips for ninja builds
f4bd91b 
Currently using the --openssl-is-fips configuration option in
combination with --ninja is broken.

This commit fixes two issues, one being an issue with the linker/version
script path variable. The second is that the locations of built
artifacts that differ for ninja and make.

ninja:
$ ./configure --openssl-is-fips --ninja
$ ninja -C out/Release
$ ./node --enable-fips -p 'crypto.getFips()'
1

make:
$ ./configure --openssl-is-fips
$ make -j8
$ ./node --enable-fips -p 'crypto.getFips()'
1

PR-URL: #40518
Refs: #40509
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
danbev added a commit that referenced this issue Oct 21, 2021
@danbev
deps: regenerate OpenSSL arch files
a16c34e 
PR-URL: #40518
Refs: #40509
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
danbev added a commit that referenced this issue Oct 21, 2021
@danbev
deps: attempt to suppress macro-redefined warning
f233cb2 
PR-URL: #40518
Refs: #40509
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
@danbev
Copy link
Contributor

danbev commented Oct 21, 2021

Closing as #40518 has landed.

@danbev danbev closed this as completed Oct 21, 2021
targos pushed a commit that referenced this issue Oct 23, 2021
@danbev @targos
deps,build,tools: fix openssl-is-fips for ninja builds
5df8ce5 
Currently using the --openssl-is-fips configuration option in
combination with --ninja is broken.

This commit fixes two issues, one being an issue with the linker/version
script path variable. The second is that the locations of built
artifacts that differ for ninja and make.

ninja:
$ ./configure --openssl-is-fips --ninja
$ ninja -C out/Release
$ ./node --enable-fips -p 'crypto.getFips()'
1

make:
$ ./configure --openssl-is-fips
$ make -j8
$ ./node --enable-fips -p 'crypto.getFips()'
1

PR-URL: #40518
Refs: #40509
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
targos pushed a commit that referenced this issue Oct 23, 2021
@danbev @targos
deps: regenerate OpenSSL arch files
d2839bf 
PR-URL: #40518
Refs: #40509
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
targos pushed a commit that referenced this issue Oct 23, 2021
@danbev @targos
deps: attempt to suppress macro-redefined warning
2b9fcdf 
PR-URL: #40518
Refs: #40509
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
branchseer added a commit to branchseer/libnode that referenced this issue Oct 23, 2021
@branchseer
Disable building with ninja
f23f277 
which fails possibly due to nodejs/node#40509
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@danbev danbev

Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@danbev @targos @gengjiawen @richardlau @Mesteery