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

Copy license file(s) to out directory #411

Merged
merged 3 commits into from
Dec 21, 2018

Conversation

mstallmo
Copy link
Member

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 for LICENSE* and copy those file(s) to the specified out directory. If a license key has not been set this step will skip with the message No 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 print License 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 for LICENSE* in the crate directory. I thought about using the actual value set in the License 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! 📦✅

  • You have the latest version of rustfmt installed and have your
    cloned directory set to nightly
$ rustup override set nightly
$ rustup component add rustfmt-preview --toolchain nightly
  • You ran rustfmt on the code base before submitting
  • You reference which issue is being closed in the PR text

✨✨ 😄 Thanks so much for contributing to wasm-pack! 😄 ✨✨

Copy link
Member

@fitzgen fitzgen left a 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!

src/license.rs Show resolved Hide resolved
src/license.rs Show resolved Hide resolved
src/license.rs Show resolved Hide resolved
src/license.rs Show resolved Hide resolved
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Thanks again @mstallmo :)

@fitzgen
Copy link
Member

fitzgen commented Oct 29, 2018

Urg, this is hitting the cargo bug on windows too.

@fitzgen
Copy link
Member

fitzgen commented Oct 30, 2018

@mstallmo I fixed the appveyor CI on master, so if you rebase, then the CI should start passing and we can merge this PR!

@mstallmo
Copy link
Member Author

Just rebased and pushed. Should be good to go shortly!

src/license.rs Outdated Show resolved Hide resolved
tests/all/utils/fixture.rs Show resolved Hide resolved
Copy link
Member

@ashleygwilliams ashleygwilliams 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 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 ashleygwilliams merged commit fa5e39b into rustwasm:master Dec 21, 2018
@mstallmo
Copy link
Member Author

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wasm-pack build should copy over the license file
3 participants