Add cli workspace crate #176

tbro · 2023-10-13T16:24:13Z

Names are provisional, please suggest better ones. rcgen's main.rs is being used as a provisional main.rs in the new crate, but will be replaced and moved to top level examples later.

Include some workspace configuration to confirm expectations
move src/main.rs to myca/src/main.rs
addresses Command line tool #175

djc

I would prefer to have a workspace-only top-level Cargo.toml and have the two crates live in separate directories.

myca feels like a too generic name, maybe rustls-cert-gen?

djc · 2023-10-13T21:33:51Z

rustls-cert-gen/src/main.rs

@@ -36,3 +36,8 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
 	fs::write("certs/key.der", &cert.serialize_private_key_der())?;
 	Ok(())
 }
+
+#[test]


This probably shouldn't get added?

Its only there to demonstrate that tests are being run on both crates. I can remove it now, but that whole file is going to get replaced in the next PR anyway.

djc

It's unfortunate that Cargo.toml -> rcgen/Cargo.toml isn't recognized as a rename by Git...

djc · 2023-10-13T21:34:18Z

rustls-cert-gen/Cargo.toml

+license.workspace = true
+edition.workspace = true
+
+[[bin]]


The [[bin]] section isn't needed -- Cargo should pick it up and do the right thing implicitly.

Names are provisional, please suggest better ones. `rcgen`'s `main.rs` is being used as a provisional `main.rs` in the new crate, but will be replaced and moved to top level examples later. * create top level virtual crate * create `rustls-cert-gen` crate and move `rcgen` to `rcgen` subfolder * Include some workspace configuration to confirm expectations * move `src/main.rs` to `rustls-cert-gen/src/main.rs` * addresses rustls#175

est31 · 2023-10-14T21:44:33Z

I would prefer to have a workspace-only top-level Cargo.toml and have the two crates live in separate directories.

Given that we only want to have two crates, I don't think that having the extra subdirectory level here makes sense. In other words, I would prefer having one Cargo.toml shared to both be workspace level and be responsible for rcgen. Edit: the binary can be in a separate directory.

tbro · 2023-10-16T16:52:39Z

I would prefer to have a workspace-only top-level Cargo.toml and have the two crates live in separate directories.

Given that we only want to have two crates, I don't think that having the extra subdirectory level here makes sense. In other words, I would prefer having one Cargo.toml shared to both be workspace level and be responsible for rcgen. Edit: the binary can be in a separate directory.

I tend to agree. It certainly makes for a smaller diff. What are your thoughts @djc ?

djc · 2023-10-17T12:53:23Z

I find Cargo manifests that mix a workspace and crate metadata very confusing, but if I'm in the minority here I don't think I get to enforce my preferences.

cpu · 2023-10-17T13:47:17Z

I find Cargo manifests that mix a workspace and crate metadata very confusing, but if I'm in the minority here I don't think I get to enforce my preferences.

I lightly prefer the workspace approach and don't think an extra subdirectory is a meaningful disadvantage (but maybe that's from bad years dealing with very deeply nested Java source trees 😬 )

tbro · 2023-10-17T17:22:35Z

Feels like opinions aren't too strong on the workspace structure question, yet a decision needs to be made. So I'm going to return to original structure of this PR (top level Cargo.toml will be responsible for both rcgen crate and workspace config). If I'm making the wrong call, we can always change it back again :).

Tinkering, I'm seeing some advantages/disadvantages. Using top level virtual crate (all crates in sub-directories) we get some neat side-effects:

cargo test from top-level will run all the tests in all the workspaces
cargo run from top-level will run the binary of rustls-cert-gen (because it is the only binary)
cargo test --all, cargo test --features x and cargo tests --no-default-features continue to work as before
happy consequence of those two is that CI and dev-x are the same as before the introduction of workspaces

With the top-level Cargo.toml managing worksapces and rcgen crate behavior changes:

cargo test from top-level now only runs rcgen tests. (b/c rcgen is assumed to be default crate)
cargo run from top-level fails (we can workaround w/ cargo run -p rustls-cert-gen
cargo test --feature x509-parser still works b/c only rcgen has this feature (but in the case it were added as a feature to rustls-cert-gen tests under rustls-cert-gen would not run (CI could be updated to make them run of course)).

Given the above, I'm leaning toward every crate in its own sub-directory. Also has the advantage that structure would not need to be changed if another crate were ever added (I have no idea if that is a serious possibility though).

cpu · 2023-10-17T19:10:56Z

Feels like opinions aren't too strong on the workspace structure question, yet a decision needs to be made

Do the advantages/disadvantages outlined by @tbro in his edited comment above sway your opinion on the subject @est31 ? If not, do you feel strongly enough to consider it blocking feedback for merging this PR? Thanks!

est31 · 2023-10-17T23:58:24Z

I'm fine with having a top-level virtual workspace it if we eliminate the src directory in both of these crates and have the .rs files at the top level, like it used to be done in rustc. It's doable by manually specifying a lib/bin section.

djc · 2023-10-18T12:41:01Z

I'm fine with having a top-level virtual workspace it if we eliminate the src directory in both of these crates and have the .rs files at the top level, like it used to be done in rustc. It's doable by manually specifying a lib/bin section.

IMO this just deviates from conventions more, making the crate harder to work on/understand for outside contributors. I honestly don't understand why you think the added hierarchy levels are such a big problem.

est31 · 2023-10-18T14:44:58Z

Extra subdirectories mean longer paths, and more things to click on in editors like vs code or such: you now have to expand the crate dir and then the src dir inside of it. the benefit is negligible because there aren't that many files in the src directory. Each time I have to expand the src dir in the editor I'm annoyed. The benefit that directories provide, bundling up things, is already there thanks to having per-crate directories.

The convention is great for projects that have a bunch of stuff like README, CI files, or license files at the top level but in multi crate projects you don't have those in each crate subdir so you don't need the src directory.

est31 · 2023-10-18T14:45:56Z

I would be okay with two options:

either having one crate at the top level also functioning as workspace root, or
having two crate directories but no separate src dir

which ones of these would you folks prefer?

tbro · 2023-10-18T16:02:01Z

The convention is great for projects that have a bunch of stuff like README, CI files, or license files at the top level but in multi crate projects you don't have those in each crate subdir so you don't need the src directory.

I think you don't have all those files in each crate, but you do have some. To me it makes sense to share a LICENSE, but not so much a README. It seems more digestible to have separate documentation. Also the published package on crates.io would need documentation.

cpu · 2023-10-18T16:26:21Z

Extra subdirectories mean longer paths, and more things to click on in editors like vs code or such: you now have to expand the crate dir and then the src dir inside of it.

My expectation is that most IDEs would preserve the folder expansion state between opening/closing the project. Certainly CLion does this. With that in mind, isn't it a one-time cost? I'm not sure I see this as a significant frustration point.

Cargo.toml

rcgen/Cargo.toml

rustls-cert-gen/Cargo.toml

* alphabetize workspace members * update `rcgen` repository url metadata * add description metadata to `rustls-cert-gen`

cpu

Thanks, LGTM.

I'll hold merge until we can confirm djc and est31 are +1 too.

est31 · 2023-10-20T19:03:29Z

I'll hold merge until we can confirm djc and est31 are +1 too.

This is a larger PR so I definitely want it to only be merged with my approval.

My expectation is that most IDEs would preserve the folder expansion state between opening/closing the project.

VS code (which I use) does preserve the open state, but it still takes away vertical space. You also have to type src/ in other settings, like for example I use the git cli for most of my git based interactions instead of the IDE, and there you'd have to type rcgen/src/lib.rs instead of rcgen/lib.rs or src/lib.rs. Sometimes I also use the github interface to interact with a repo, e.g. to browse the tree at a certain commit without touching my local checkout. There too the additional nesting just makes things worse. So it's not really a one time cost but one you pay over and over again.

That's why I deviate from the convention here, and the benefits of the convention are only minimal. Very often I've visited projects and opened the Cargo.toml just to discover that it is a workspace top level toml and I had to guess where the main crate of the project was. In fact, this is one of the least standardized areas of Rust, some people make a crates/ directory, others have a lib directory, in multi language projects you can find rust directories. I don't want others to get a similar experience when dealing with rcgen. For a git repo named rcgen, it's reasonable to assume that the top level src directory is for the rcgen crate.

In other words, I don't think the currently proposed directory layout is good. The two layouts I'd be okay with I've outlined in #176 (comment) .

Should we ever have 4-5+ crates in rcgen, and a 50+ line workspace top level section, we can rethink that choice, but for now I would really prefer having the src/ dir for rcgen at the top level.

Also, looking at this PR, Cargo.lock should definitely not exist in the rcgen subdirectory, nor should codecov.yml. For README, I don't think we need one specific to rcgen either (we could have one specific to the cli tool tho).

cpu · 2023-10-20T20:07:44Z

This is a larger PR so I definitely want it to only be merged with my approval.

SGTM.

There too the additional nesting just makes things worse. So it's not really a one time cost but one you pay over and over again.

I think there are strategies to overcome all of these issues (tab completion, using fuzzy file finders, various shortcuts, etc). It still feels to me like a minor issue and not strong justification for why we should do something unique compared to what I see other multi-crate workspaces doing.

In other words, I don't think the currently proposed directory layout is good. The two layouts I'd be okay with I've outlined in #176 (comment) .

This feels like a place where there's consensus among @tbro, @djc and myself that the current layout implemented in-branch is OK. It gives me pause for concern that you want to override that consensus, especially for something relatively small in nature. If in the future there are larger points of contention (particularly in the domain of Rust code as opposed to the project layout) would consensus be overruled again? I can accept one of your proposed alternatives if you feel very strongly, but (respectfully) I do worry that this indicates you want to continue operating as if this is your project and not a community effort.

est31 · 2023-10-20T21:35:59Z

I agree that it is a minor issue and if you folks really insist, we can have it the "conventional" way, not because you managed to convince me that it is a better solution technically but because of social reasons (I value you a lot as co-maintainers). The Cargo.lock, codecov yaml and README should definitely be deleted from the rcgen dir though.

Note that I have never really believed in following conventions unless they deliver explicit value. This is probably why I created rcgen in the first place: the convention would have been to use openssl instead of reading the spec and implementing it myself in a library. You know the "don't run your own crypto" saying...

I think since rcgen's creation, the gathering momentum behind rust has attracted attention from increasingly serious people. I suppose these are the folks you @cpu see as the future, currently underserved, potential rcgen users, that you want to attract. My personal use cases were quite non-serious (I just wanted something that outputs something I can feed to quic libraries that forces usage of TLS). Now in 2023 it's way less of a weird choice to pick Rust for some project than it was in 2019.

But while the community grew, I haven't changed. To me, following conventions is just as weird as it was in 2019, although I think I do have gained a larger appreciation for social cohesion in that time period. This non-following of conventions is partially driven by my desire to use my projects as a way to move conventions forward and improve them, but also because I want to do the technically best thing.

I'm typing this precisely because of the concerns you expressed above about the potential disagreements we could have about future Rust changes. I think some of them can also be seen in the "following conventions" vs "trying to set trends"/"doing something tailored for a small target group" angle.

I definitely want to avoid some forking event i.e. what happened to webpki, and therefore am fine with even more collaboration than what I originally thought about #137. Doesn't mean that I will stop saying my opinion though :).

cpu · 2023-10-21T13:38:17Z

Thanks for writing that out @est31. I really appreciate you sharing your opinions and don't want that to stop 👍 In my mind having these discussions seems like a healthy part of a navigating the evolution of an established project.

I also think it's a good practice to question why we're doing something and consider alternatives. I'm definitely not arguing for blindly taking all convention - I'm always open for discussion.

I think when you're working on a project in isolation it's easier to discard convention without increasing barriers to contribution, but when there's more interest in getting help from a broader audience the calculus can change. In my experience the conventions make it easier to move between projects without as much friction. That sentiment is maybe hard to quantify but does feel like explicit value to me, especially as someone trying to contribute across many repositories in a given week. I think the other aspect is that conventions are well known, but your preferences aren't. I think we'll get a better sense of your preferences over time but right now we sort of bump into them on a case-by-case basis more unexpectedly.

You're right that I'm interested in a more serious direction (though maybe we have different ideas of what serious means :-)). I think you're also right that historically I've been more interested in serving the majority use-case vs making sure other use-cases for smaller groups of enthusiasts are served. With that said like you I'm also interested in making this a collaboration that works for everyone. I think having your perspective as a counter balance to mine is very helpful. The world is better for everyone if we can find solutions that help with both sets of users and I'm not unwilling to meet somewhere in the middle.

Back to the PR at hand 😅 ...

I agree that it is a minor issue and if you folks really insist, we can have it the "conventional" way

For a small issue, my gut says we should go with the majority if you're willing to bend here. I appreciate that you're making a compromise and I hope we can make the need for such a rare event. I also see it as a great reminder for why I should compromise on some things I might have taken a harder stance on in the future. It should be a two way street - so please call me out if I don't remember that.

The Cargo.lock, codecov yaml and README should definitely be deleted from the rcgen dir though.

That sounds uncontroversial to me if it doesn't break something 👍

Note that top-level `Cargo.lock` had been auto-generated in this PR, changing the versions of some dependencies. So I also reverted that file to the versions found in `rcgen/Cargo.lock`. * move `rcgen/README.md` up a level * remove `rcgen/Cargo.lock` * revert `Cargo.lock` to previous versions * move `codecov.yml` up a level

tbro · 2023-10-24T17:55:37Z

The Cargo.lock, codecov yaml and README should definitely be deleted from the rcgen dir though.

That sounds uncontroversial to me if it doesn't break something 👍

Those have been moved as requested. Are there any other pending issues with this PR?

cpu

LGTM!

est31 · 2023-10-25T01:58:41Z

rcgen/Cargo.toml

+edition.workspace = true
+keywords = ["mkcert", "ca", "certificate"]
+
+[lib]


this section isn't needed, cargo picks it up automatically.

est31 · 2023-10-25T01:59:02Z

rcgen/Cargo.toml

+name = "rcgen"
+version = "0.11.3"
+description = "Rust X.509 certificate generator"
+repository = "https://github.com/rustls/rcgen"


can this be a workspace-level thingy as well?

name cannot. I moved the rest in latest commit. I also moved keywords up a level. reference for keys allowed in workspace.package: https://doc.rust-lang.org/cargo/reference/workspaces.html#the-package-table

est31

LICENSE is still missing in the new tool's directory.

Also wondering about the CI. Have you looked over the file and verified that all the commands work as they did before?

* copy LICENSE to `rustl-cert-gen` * move more metadata to workspace level * remove `[lib]` section from `rcgen/Cargo.toml``

tbro · 2023-10-25T15:28:46Z

LICENSE is still missing in the new tool's directory.

I copied it to rustls-cert-gen crate in latest commit. I think the workspace inheritance was doing this for us anyway, but I guess its best to cover all the bases. Question, since we are including LICENSE in the repo, should we be using license_file key instead of license? https://doc.rust-lang.org/cargo/reference/manifest.html#the-license-and-license-file-fields.

Also wondering about the CI. Have you looked over the file and verified that all the commands work as they did before?

I have. Basically, cargo commands will iterate all the crates of the workspace, so CI commands continue to work as before. There are some details in this comment: #176 (comment)

est31 · 2023-10-25T16:21:45Z

since we are including LICENSE in the repo, should we be using license_file key instead of license

this breaks many tools that do license based validation. having a field that is easily parseable is better for them. I asked for the file to be copied so that it ends up in the package.

cpu · 2023-10-25T16:47:10Z

@tbro Thanks for sticking with this. I appreciate it!

tbro · 2023-10-25T17:01:59Z

@tbro Thanks for sticking with this. I appreciate it!

Threaded the needle! ;) now on to the next one 😅

This branch adds basic support emitting and parsing distinguished name values that are Ia5Strings. For example, email address attributes in a certificate subject distinguished name. Note that because of #181 this code will panic when emitting invalid Ia5String values. This problem is general to rcgen's handling of ASN.1 string types and so isn't addressed with additional care in this branch. A broader rework is required. Along the way I also fixed a warning from #176 related to where we were defining the custom `profile.dev.package.num-bigint-dig` profile metadata. Resolves #180

djc reviewed Oct 13, 2023

View reviewed changes

tbro force-pushed the init-cli-crate branch 2 times, most recently from cdb830c to b757413 Compare October 13, 2023 20:32

tbro marked this pull request as ready for review October 13, 2023 20:35

tbro requested a review from djc October 13, 2023 20:35

djc reviewed Oct 13, 2023

View reviewed changes

tbro force-pushed the init-cli-crate branch from 63bbbf7 to d2a8ea7 Compare October 14, 2023 17:43

cpu approved these changes Oct 19, 2023

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

rcgen/Cargo.toml Outdated Show resolved Hide resolved

rustls-cert-gen/Cargo.toml Show resolved Hide resolved

Address latest review comments

5caaf74

* alphabetize workspace members * update `rcgen` repository url metadata * add description metadata to `rustls-cert-gen`

tbro requested a review from cpu October 19, 2023 17:04

cpu approved these changes Oct 19, 2023

View reviewed changes

tbro requested review from cpu and djc October 23, 2023 15:23

cpu approved these changes Oct 24, 2023

View reviewed changes

est31 reviewed Oct 25, 2023

View reviewed changes

est31 requested changes Oct 25, 2023

View reviewed changes

Fix review comments

079e2d6

* copy LICENSE to `rustl-cert-gen` * move more metadata to workspace level * remove `[lib]` section from `rcgen/Cargo.toml``

tbro requested a review from est31 October 25, 2023 15:48

est31 approved these changes Oct 25, 2023

View reviewed changes

est31 added this pull request to the merge queue Oct 25, 2023

Merged via the queue into rustls:main with commit 2d09fd3 Oct 25, 2023
15 checks passed

tbro deleted the init-cli-crate branch October 25, 2023 16:53

cpu mentioned this pull request Oct 27, 2023

basic Ia5String support for DistinguishedName values #182

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cli workspace crate #176

Add cli workspace crate #176

tbro commented Oct 13, 2023

djc left a comment

djc Oct 13, 2023

tbro Oct 13, 2023

djc left a comment

djc Oct 13, 2023

est31 commented Oct 14, 2023 •

edited

Loading

tbro commented Oct 16, 2023

djc commented Oct 17, 2023

cpu commented Oct 17, 2023

tbro commented Oct 17, 2023 •

edited

Loading

cpu commented Oct 17, 2023

est31 commented Oct 17, 2023

djc commented Oct 18, 2023

est31 commented Oct 18, 2023

est31 commented Oct 18, 2023 •

edited

Loading

tbro commented Oct 18, 2023 •

edited

Loading

cpu commented Oct 18, 2023

cpu left a comment

est31 commented Oct 20, 2023

cpu commented Oct 20, 2023 •

edited

Loading

est31 commented Oct 20, 2023

cpu commented Oct 21, 2023

tbro commented Oct 24, 2023

cpu left a comment

est31 Oct 25, 2023

est31 Oct 25, 2023

tbro Oct 25, 2023

est31 left a comment

tbro commented Oct 25, 2023

est31 commented Oct 25, 2023

cpu commented Oct 25, 2023

tbro commented Oct 25, 2023

Add cli workspace crate #176

Add cli workspace crate #176

Conversation

tbro commented Oct 13, 2023

djc left a comment

Choose a reason for hiding this comment

djc Oct 13, 2023

Choose a reason for hiding this comment

tbro Oct 13, 2023

Choose a reason for hiding this comment

djc left a comment

Choose a reason for hiding this comment

djc Oct 13, 2023

Choose a reason for hiding this comment

est31 commented Oct 14, 2023 • edited Loading

tbro commented Oct 16, 2023

djc commented Oct 17, 2023

cpu commented Oct 17, 2023

tbro commented Oct 17, 2023 • edited Loading

cpu commented Oct 17, 2023

est31 commented Oct 17, 2023

djc commented Oct 18, 2023

est31 commented Oct 18, 2023

est31 commented Oct 18, 2023 • edited Loading

tbro commented Oct 18, 2023 • edited Loading

cpu commented Oct 18, 2023

cpu left a comment

Choose a reason for hiding this comment

est31 commented Oct 20, 2023

cpu commented Oct 20, 2023 • edited Loading

est31 commented Oct 20, 2023

cpu commented Oct 21, 2023

tbro commented Oct 24, 2023

cpu left a comment

Choose a reason for hiding this comment

est31 Oct 25, 2023

Choose a reason for hiding this comment

est31 Oct 25, 2023

Choose a reason for hiding this comment

tbro Oct 25, 2023

Choose a reason for hiding this comment

est31 left a comment

Choose a reason for hiding this comment

tbro commented Oct 25, 2023

est31 commented Oct 25, 2023

cpu commented Oct 25, 2023

tbro commented Oct 25, 2023

est31 commented Oct 14, 2023 •

edited

Loading

tbro commented Oct 17, 2023 •

edited

Loading

est31 commented Oct 18, 2023 •

edited

Loading

tbro commented Oct 18, 2023 •

edited

Loading

cpu commented Oct 20, 2023 •

edited

Loading