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

Automatically read license field from Cargo.toml #2144

Merged
merged 1 commit into from
Dec 8, 2020
Merged

Automatically read license field from Cargo.toml #2144

merged 1 commit into from
Dec 8, 2020

Conversation

robinmoussu
Copy link
Contributor

Note: this is my first time contributing to clap, and I'm not familiar with the codebase. Please review that I didn't forgot anything.

I recently added support in Cargo for the environment variable CARGO_PKG_LICENSE_FILE in addition to the recently added CARGO_PKG_LICENSE. This commit automatically populate the license field by reading those environment variable.

If both of the environment variable are empty, should we panic!() instead of just returning an empty string? I don't think that panicking is a good idea, since it would possibly break crates that are using the app_from_crate! macro, but I don't know of a better way to add a warning.

This may help a bit with #1768.

@robinmoussu
Copy link
Contributor Author

I realized that this PR bump the MSRV to 1.46 or later (since the environment variables where added in this release). Should I use option_env! instead of the env! macro?

pksunkara
pksunkara requested changes Sep 25, 2020
View reviewed changes
Copy link
Member

@pksunkara pksunkara left a comment

Choose a reason for hiding this comment

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

Why would this bump MSRV?

Also, please add some tests.

src/build/app/mod.rs Show resolved Hide resolved
@robinmoussu
Copy link
Contributor Author

Why would this bump MSRV?

Because cargo < 1.46 doesn't set CARGO_PKG_LICENSE_FILE, likewise cargo < 1.45 for CARGO_PKG_LICENSE. If the environment variable isn't set, the env! macro fails at compile time. This means that this commit requires a MSRV of 1.46 or later as you can see in the results of the CI. If bumping the MSRV is not desirable, it's also possible to use option_env! instead of env! since the former returns None when the environment variable isn't set, while the later panics.

Looking at the Readme, I found that:

clap will officially support current stable Rust, minus two releases, but may work with prior releases as well.

So I think that we should either wait for 2 additional stable releases of rust, or change env! by option_env! before merging this commit.

Also, please add some tests.

I tried to look at the history to know where I should add them, but unfortunately when the macro crate_description!, crate_name! and app_from_crate! were introduced in 4d9a82d no tests where added. Could you please help me to find where I should add them?

@pksunkara pksunkara marked this pull request as draft September 27, 2020 21:21
@pksunkara
Copy link
Member

pksunkara commented Sep 29, 2020 •
edited
Loading

I have created "Rust 1.46" label so that we can look at the issues for it later.

@robinmoussu
Copy link
Contributor Author

Good idea

@pksunkara pksunkara marked this pull request as ready for review November 28, 2020 13:17
pksunkara
pksunkara requested changes Nov 28, 2020
View reviewed changes
Copy link
Member

@pksunkara pksunkara left a comment •
edited
Loading

Choose a reason for hiding this comment

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

So, we are setting the license, but where exactly is being used?

You also need to rebase on master now.

examples/08_subcommands.rs Outdated Show resolved Hide resolved
src/macros.rs Outdated Show resolved Hide resolved
src/macros.rs Outdated Show resolved Hide resolved
@pksunkara
Copy link
Member

@robinmoussu Do you plan to pick this up?

@robinmoussu
Copy link
Contributor Author

Yes. Sorry the delay. I should have the time this evening.

@robinmoussu
Copy link
Contributor Author

So, we are setting the license, but where exactly is being used?

I didn't realized that it's possible to specify the license (multiple tests where already doing it), but it doesn't seems possible to use it. This is especially weird in new_help.rs where a --license argument is created, but it doesn't have any actions associated (it was modified in 0333380).

pksunkara
pksunkara requested changes Dec 8, 2020
View reviewed changes
Copy link
Member

@pksunkara pksunkara left a comment

Choose a reason for hiding this comment

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

You haven't added a test for crate_license which would have caught the issue I mentioned in the other comment. Please do.

src/macros.rs Outdated Show resolved Hide resolved
@robinmoussu
Copy link
Contributor Author

You haven't added a test for crate_license which would have caught the issue I mentioned in the other comment. Please do.

I am relatively surprised, and I think I totally forgot the context of when I wrote this PR. I thought that the notion of "license" was already exposed in clap, but it seems not. I'm working on it.

@robinmoussu
Copy link
Contributor Author

This is a much bigger change than what I expected. And I will not have the time to do it short term. I will finish it (unless someone does it before me) probably in a few months when my current project will no longer have high priority. Here is a list of what is missing:

  • adding the version in the output of --help and --version (like what ls does)
  • automatically add an option --license that displays the license if the license is provided and the option doesn't exists (like --help and --version)
  • if a license file is used, I think its content should be embeded it into the binary, or at least the first line/paragraph to be able to display it correctly (in case the file is named something like "LICENSE.txt").
  • make it possible to add a license for every supported way in clap (like with the yaml parser, more or less here).

@pksunkara pksunkara merged commit 0a91329 into clap-rs:master Dec 8, 2020
@ErichDonGubler ErichDonGubler mentioned this pull request Oct 30, 2021
Draft
1 task
@epage epage mentioned this pull request Nov 8, 2021
Closed
2 tasks
epage added a commit to epage/clap that referenced this pull request Nov 12, 2021
@epage
Revert "Automatically read license field from Cargo.toml"
7a59dc3 
This reverts commit 6898fbd.

PR clap-rs#2144 added the `license` field but no consumer has been added since
the (like Issue clap-rs#1768).  Since this is not ready yet, I am pulling it
from the 3.0 release.

So far, our main route for pulling a feature from the release has
been to put it behind a `unstable-*` feature flag and to create a
stablization tracking issue.  I chose to instead remove the feature
because a write-only field with no effect does not provide values for
people to use in as an early access and so doesn't outweight the cost of
the extra documentation noise and code noise it creates.  Additionally,
keeping an `unstable-` feature around when it has such an unknown path
(and time table) to stalbization feels like it violates YAGNI.  I'm
uncertain how much of this feature we can implement and not create a
legal trap for users because the crate's license is insufficient for the
final artifact's license.  I feel our stabliazation process sshould be
about iteration and collecting user feedback which this doesn't line up
with.

When someone is ready to tackle clap-rs#1768, it will be easy to revert this
commit and pick up the work again.

Fixes clap-rs#3001
@epage epage mentioned this pull request Nov 12, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pksunkara pksunkara pksunkara requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@robinmoussu @pksunkara