-
Notifications
You must be signed in to change notification settings - Fork 409
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
Copy license file(s) to out directory #411
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice suite of tests!
I have a few nitpicks inline below, but once we address those, this should be good to merge. Thanks @mstallmo!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again @mstallmo :)
Urg, this is hitting the cargo bug on windows too. |
@mstallmo I fixed the appveyor CI on master, so if you rebase, then the CI should start passing and we can merge this PR! |
Just rebased and pushed. Should be good to go shortly! |
Check the Cagro.toml for a license and if one is found glob for LICENSE* files to copy to the out directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this looks great! thanks so much- i'm a little worried about some of the unwraps in here, but i think that's something we can refactor later on. thank you so much!
@ashleygwilliams thanks for the merge! I'll start on the refactor for this over the next couple of weeks. I'm off work for the holidays so I'll have a some good free time to focus on OSS stuff! |
Closes #407
Added a new step to
build
that will look in the Cargo.toml file to see if a license key has been set. If one has been set it will glob forLICENSE*
and copy those file(s) to the specifiedout
directory. If a license key has not been set this step will skip with the messageNo LICENSE found in Cargo.toml skipping...
. If a key is set in the Cargo.toml but the glob doesn't match any files we will break out of the copying license step and printLicense key is set in Cargo.toml but no LICENSE file(s) were found; Please add the LICENSE file(s) to your project directory
to the console.I would love some feed back on the file globing implementation. I am pretty new to Rust but as far as I can tell the best way to account for the different ways
LICENSE
files can be named was to implement a glob that looks forLICENSE*
in the crate directory. I thought about using the actual value set in theLicense
key in the Cargo.toml but due to the non-standard ways licenses can be referred to I figured it would be best to just check for it's existence rather than relying on the value set for the key itself. This seemed like the most robust way to go about this but I would love to know if there is a better way that I'm not aware of!I know it's common in the Rust community to dual license Apache 2.0 and MIT but since that pattern is less common in the JS community I'm not sure if we would want give the ability for users to select which license they want copied if more than one is found. Related, I'm not even sure if it would be desired or appropriate for a user to change the licensing of the wasm-packed output as compared to the Rust source code but it was just a thought I had while implementing this feature.
Make sure these boxes are checked! 📦✅
rustfmt
installed and have yourcloned directory set to nightly
$ rustup override set nightly $ rustup component add rustfmt-preview --toolchain nightly
rustfmt
on the code base before submitting✨✨ 😄 Thanks so much for contributing to wasm-pack! 😄 ✨✨