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

move CCADB fetching into a crate #56

Merged
merged 2 commits into from
Dec 6, 2023
Merged

move CCADB fetching into a crate #56

merged 2 commits into from
Dec 6, 2023

Conversation

mspiegel
Copy link
Contributor

@mspiegel mspiegel commented Nov 29, 2023

Change the webpki-roots repo to be a workspace that includes a crate that pulls the CCADB stuff and exposes an API. This pull request has a dependency on rustls/webpki#212.

@cpu
Copy link
Member

cpu commented Nov 29, 2023

Thanks for the PR! Some initial feedback:

  • Based on experience with a similar change in another Rustls repo I think we would prefer to have a workspace-only top-level Cargo.toml and have the two crates live in separate directories.
  • While waiting on the webpki fix for the pki-types semver break I think it would be better to leave this repo using alpha.7, and change the pki-types dependencies from 0.2.2 to =0.2.2. That should unbreak the build/CI - probably best done in its own commit ahead of the other work.
  • Relatedly, pki-types should probably be a workspace dependency. We'd want both crates to use the same version.
  • webpki_ccadb::fetch_ccadb_roots could use a Rustdoc comment since it's now public API surface.
  • I think it would be nice to update README.md to mention the webpki-ccadb crate.
  • Please don't reference GitHub issues in commit messages (e.g. b8ae94b) - this is mentioned in CONTRIBUTING.md in "code changes".

@cpu
Copy link
Member

cpu commented Nov 29, 2023

incorporating feedback from code review

It would be easier to review if you could keep the commit history tidy, with discrete commits instead of layering the feedback on top in one commit. Thank you

@djc
Copy link
Member

djc commented Nov 29, 2023

The first commit here should turn the repo into a Cargo workspace without doing anything else, to untangle that change from everything else. The next commit should do the minimal possible change to move code from codegen.rs into the new crate's lib.rs. Changes like alphabetizing dependencies should not appear in a commit if the ordering issue was introduced in an earlier commit.

(There's also no need to pin the pki-types dependency to =0.2.2 -- just 0.2.2 should suffice.)

@cpu
Copy link
Member

cpu commented Nov 29, 2023

(There's also no need to pin the pki-types dependency to =0.2.2 -- just 0.2.2 should suffice.)"

Sorry this one is on me: I accidentally broke semver w/ 0.2.3 and webpki won't build w/ 0.2.3 without landing rustls/webpki#212

@cpu
Copy link
Member

cpu commented Nov 29, 2023

Sorry this one is on me: I accidentally rustls/pki-types#14 (comment) w/ 0.2.3

This is resolved w/ webpki 0.102.0-alpha.8

@mspiegel
Copy link
Contributor Author

I'll update the commit history to have 2 commits as suggested and use webpki 0.102.0-alpha.8.

@mspiegel
Copy link
Contributor Author

I've updated the commit history. Let me know if I should make any changes.

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

This is looking pretty good. If you rebase this (instead of adding a merge commit) that should obviate the need for the webpki alpha bump.

Cargo.toml Outdated Show resolved Hide resolved
webpki-ccadb/Cargo.toml Outdated Show resolved Hide resolved
@mspiegel mspiegel force-pushed the ccadb-crate branch 6 times, most recently from eea7a22 to b7b5817 Compare December 1, 2023 15:26
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
webpki-roots/Cargo.toml Outdated Show resolved Hide resolved
@mspiegel mspiegel force-pushed the ccadb-crate branch 4 times, most recently from d5ed3c7 to 17c3993 Compare December 1, 2023 16:23
@mspiegel
Copy link
Contributor Author

mspiegel commented Dec 1, 2023

What license should be assigned to the webpki-ccadb crate? The webpki-ccadb crate contains source code that was moved from the webpki-roots crate. The webpki-roots crate is under "MPL-2.0". Therefore webpki-ccadb should also be "MPL-2.0"?

@mspiegel
Copy link
Contributor Author

mspiegel commented Dec 4, 2023

I've added the MPL 2.0 license to the webpki-ccadb crate. Let me know if any more changes are needed.

@mspiegel mspiegel requested a review from djc December 4, 2023 15:55
LICENSE Outdated Show resolved Hide resolved
webpki-roots/Cargo.toml Show resolved Hide resolved
webpki-roots/Cargo.toml Show resolved Hide resolved
@mspiegel mspiegel force-pushed the ccadb-crate branch 2 times, most recently from ef7a640 to 674af11 Compare December 5, 2023 14:15
@ctz
Copy link
Member

ctz commented Dec 5, 2023

What license should be assigned to the webpki-ccadb crate? The webpki-ccadb crate contains source code that was moved from the webpki-roots crate. The webpki-roots crate is under "MPL-2.0". Therefore webpki-ccadb should also be "MPL-2.0"?

The webpki-roots crate is distributed under MPL-2.0 because it was an automatically generated derived work from an MPL-2.0-licensed source.

That doesn't really apply to the code we (predominantly, @cpu) wrote which would form the basis of this new library crate. I'd suggest MIT OR Apache-2.0 would be the most natural choice.

@cpu
Copy link
Member

cpu commented Dec 5, 2023

I'd suggest MIT OR Apache-2.0 would be the most natural choice.

SGTM.

@mspiegel
Copy link
Contributor Author

mspiegel commented Dec 5, 2023

What license should be assigned to the webpki-ccadb crate? The webpki-ccadb crate contains source code that was moved from the webpki-roots crate. The webpki-roots crate is under "MPL-2.0". Therefore webpki-ccadb should also be "MPL-2.0"?

The webpki-roots crate is distributed under MPL-2.0 because it was an automatically generated derived work from an MPL-2.0-licensed source.

That doesn't really apply to the code we (predominantly, @cpu) wrote which would form the basis of this new library crate. I'd suggest MIT OR Apache-2.0 would be the most natural choice.

The webpki-ccadb crate is source code that was in the webpki-roots crate. All the source code of the webpki-roots crate was under the MPL 2.0 license. Can I get confirmation that you wish to change the license of webpki-ccadb to be 'MIT or APACHE-2.0'?

@mspiegel
Copy link
Contributor Author

mspiegel commented Dec 5, 2023

Sorry, I didn't see @cpu's affirmation above. I will change the license.

@mspiegel
Copy link
Contributor Author

mspiegel commented Dec 5, 2023

Changed license of webpki-ccadb to be "MIT or APACHE-2.0"

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

One last nit, will be good to merge this.

the [webpki](https://github.com/rustls/webpki) or
[rustls](https://github.com/rustls/rustls) crates.

This crate is inspired by [certifi.io](https://certifi.io/en/latest/) and
Copy link
Member

Choose a reason for hiding this comment

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

This is removed only to be brought back (in slightly modified form) in the next commit. Let's leave it here in the first place? Sorry, missed this in the previous round due to the other things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made the change to keep the paragraph that starts with 'The webpki-roots crate is inspired...' and all the paragraphs below it into the top-level README.md. Did you want only the paragraph 'The webpki-roots crate is inspired by...' to remain in the top-level README.md or that paragraph and the ones below it as well?

Change the webpki-roots repo to be a workspace that includes a crate
that pulls the CCADB stuff and exposes an API.
@djc djc merged commit 0df3d50 into rustls:main Dec 6, 2023
1 check passed
@djc
Copy link
Member

djc commented Dec 7, 2023

I've published the webpki-ccadb 0.1.0 release to crates.io.

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.

4 participants