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: Use system NSS when possible #1739

Merged
merged 50 commits into from
Mar 27, 2024

Conversation

larseggert
Copy link
Collaborator

@larseggert larseggert commented Mar 13, 2024

When NSS_DIR is not set, we now attempt to use the system NSS, and check whether it is recent enough for us. The current NSS minimum version is in neqo-crypto/min_version.txt, so it can be accessed by Rust (via neqo-crypto/src/min_version.rs) and by the CI scripts.

Fixes #1711

neqo-crypto/build.rs Outdated Show resolved Hide resolved
neqo-crypto/build.rs Outdated Show resolved Hide resolved
Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that you can cut most of fn nss_dir() now that you won't be hitting it as often. Just pass the env variable in to setup_standalone()

.github/actions/nss/action.yml Outdated Show resolved Hide resolved
.github/actions/nss/action.yml Show resolved Hide resolved
neqo-crypto/build.rs Outdated Show resolved Hide resolved
neqo-crypto/build.rs Outdated Show resolved Hide resolved
neqo-crypto/build.rs Outdated Show resolved Hide resolved
larseggert and others added 3 commits March 19, 2024 01:57
Co-authored-by: Martin Thomson <mt@lowentropy.net>
Signed-off-by: Lars Eggert <lars@eggert.org>
Co-authored-by: Martin Thomson <mt@lowentropy.net>
Signed-off-by: Lars Eggert <lars@eggert.org>
Co-authored-by: Martin Thomson <mt@lowentropy.net>
Signed-off-by: Lars Eggert <lars@eggert.org>
@larseggert
Copy link
Collaborator Author

larseggert commented Mar 19, 2024

I think that you can cut most of fn nss_dir() now that you won't be hitting it as often. Just pass the env variable in to setup_standalone()

nss_dir() has the code that checks out NSS and NSPR if there is no copy already. Are you saying that should be cut, i.e., will we require people to do their own checkouts?

Ping @martinthomson

Not ready yet. Need to determine what to do in `nss_dir()`. See comments.
larseggert added a commit to larseggert/neqo that referenced this pull request Mar 19, 2024
@larseggert larseggert linked an issue Mar 19, 2024 that may be closed by this pull request
neqo-crypto/build.rs Outdated Show resolved Hide resolved
neqo-crypto/build.rs Outdated Show resolved Hide resolved
neqo-crypto/src/lib.rs Outdated Show resolved Hide resolved
neqo-crypto/min_version.rs Outdated Show resolved Hide resolved
.github/actions/nss/action.yml Show resolved Hide resolved
@larseggert larseggert added this pull request to the merge queue Mar 26, 2024
@martinthomson martinthomson removed this pull request from the merge queue due to a manual request Mar 26, 2024
@martinthomson
Copy link
Member

OK, so approvals with comments get merged. That's not ideal.

@martinthomson
Copy link
Member

@larseggert should we require that conversations are resolved before merging?

image

@larseggert
Copy link
Collaborator Author

@martinthomson we can turn that on, but then we should probably turn off Require approval of the most recent reviewable push, because otherwise a re-review would be required just as it is now if one had chosen "request changes"?

larseggert and others added 2 commits March 26, 2024 09:01
Co-authored-by: Martin Thomson <mt@lowentropy.net>
Signed-off-by: Lars Eggert <lars@eggert.org>
@larseggert
Copy link
Collaborator Author

I think that you can cut most of fn nss_dir() now that you won't be hitting it as often. Just pass the env variable in to setup_standalone()

@martinthomson: nss_dir() has the code that checks out NSS and NSPR if there is no copy already. Are you saying that should be cut, i.e., will we require people to do their own checkouts?

@martinthomson
Copy link
Member

turn off Require approval of the most recent reviewable push

Yeah, I think that we can trust people to exercise their judgment on the small stuff.

will we require people to do their own checkouts?

Yeah. You will observe that you can't reach that code unless you set NSS_DIR but haven't got a copy of NSS. Given that the main reason to use NSS_DIR is to point to a copy you are working on, that seems like a good trade to me.

@larseggert larseggert added this pull request to the merge queue Mar 27, 2024
Merged via the queue into mozilla:main with commit 47dfb3b Mar 27, 2024
14 checks passed
@larseggert larseggert deleted the build-use-system-nss branch March 27, 2024 08:10
github-merge-queue bot pushed a commit that referenced this pull request Mar 27, 2024
* feat: Turn on TLS greasing

Fixes #1391
Needs #1739

* Make clippy happy
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.

Use system-installed NSS dependeny Can Neqo use NSS system libs on Linux?
2 participants