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: update with changes for OpenSSL 3.x #149

Closed
wants to merge 4 commits into from

Conversation

danbev
Copy link

@danbev danbev commented May 9, 2022

This commit updates init.py with changes that were made directly
in Node.js (sorry about this but at the time I was not aware that there
this upstream repo existed).

It includes changes from two commits in Node.js:
Commit f4bd91b0e2d7f07365ccf6bba943ba97d6f95cc9 ("deps,build,
tools: fix openssl-is-fips for ninja builds) and
Commit 66da32c045035cf2710a48773dc6f55f00e20c40 ("deps,test,src,doc,
tools: update to OpenSSL 3.0")

Refs: nodejs/node#40735

This commit updates __init__.py with changes that were made directly
in Node.js (sorry about this but at the time I was not aware that there
this upstream repo existed).

It includes changes from two commits in Node.js:
 Commit f4bd91b0e2d7f07365ccf6bba943ba97d6f95cc9 ("deps,build,
tools: fix openssl-is-fips for ninja builds) and
Commit 66da32c045035cf2710a48773dc6f55f00e20c40 ("deps,test,src,doc,
tools: update to OpenSSL 3.0")

Refs: nodejs/node#40735
@danbev
Copy link
Author

danbev commented May 9, 2022

This change is also available in #147 but I was not sure if you preferred to have a separate commit. Also sorry about not making the changes upstream first, it was just lack of knowledge on my part about how project is used by Node.js.

pylib/gyp/__init__.py Outdated Show resolved Hide resolved
@gengjiawen
Copy link
Member

Is changes pylib/gyp/generator/make.py no longer needed ?

pylib/gyp/__init__.py Outdated Show resolved Hide resolved
@danbev
Copy link
Author

danbev commented May 11, 2022

Is changes pylib/gyp/generator/make.py no longer needed ?

I'll take a closer look but just by looking at them I really don't recognize those changes, but then again it was a long time since they were made.

@gengjiawen
Copy link
Member

Is changes pylib/gyp/generator/make.py no longer needed ?

I'll take a closer look but just by looking at them I really don't recognize those changes, but then again it was a long time since they were made.

comes from nodejs/node#38512

@danbev
Copy link
Author

danbev commented May 11, 2022

comes from nodejs/node#38512

Thanks, I saw that it was included by that commit, but what I can't remember is the reason for these changes. I have a vague memory that this was a change during early development where there were some command line options that became too long (something like that). I've tried reverting the changes to make.py and running the Node.js CI tests to see if anything breaks: nodejs/node#43051.

The above CI run failed and the error was:

08:09:48 make[2]: execvp: printf: Argument list too long
08:09:48 deps/openssl/openssl.target.mk:1205: recipe for target '/home/iojs/build/workspace/node-test-commit-linux-containered/out/Release/obj.target/deps/openssl/libopenssl.a' failed
08:09:48 make[2]: *** [/home/iojs/build/workspace/node-test-commit-linux-containered/out/Release/obj.target/deps/openssl/libopenssl.a] Error 127

So this will include all the changes in #147 so I'm going to close this.

@targos
Copy link
Member

targos commented May 11, 2022

To make the changelog entry clearer about the change, what commit message should we use?

I'd start with this:

feat: add `PRODUCT_DIR_ABS` variable

@targos
Copy link
Member

targos commented May 11, 2022

It would be nice to explain what directory the variable points to and what it can be used for.

@danbev danbev closed this May 11, 2022
@targos
Copy link
Member

targos commented May 11, 2022

Why close this? I thought it made sense to separate the __init__ and make changes to better document them in the changelog.

@danbev
Copy link
Author

danbev commented May 11, 2022

Why close this? I thought it made sense to separate the __init__ and make changes to better document them in the changelog.

My reason for opening this PR was I did not think all the changed in the other PR were required, but I was wrong about that. If you want to split #147 in multiple commits then that sounds good to me.

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.

5 participants