-
Notifications
You must be signed in to change notification settings - Fork 361
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
Update WASM binary and Land WASM rust sources in this repository #465
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.
RSLGTM on the Rust and Python code (assuming that it's just copying existing work). Left some questions in metadata files and license updates.
wasm-mappings/.gitattributes
Outdated
@@ -0,0 +1,2 @@ | |||
README.md -diff -merge |
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.
Is this redundant with the top-level attributes?
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.
Oh yes I forgot to remove it after folding it into the main one.
wasm-mappings/.travis.yml
Outdated
@@ -0,0 +1,27 @@ | |||
language: rust |
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.
Should this be removed?
wasm-mappings/LICENSE-APACHE
Outdated
@@ -0,0 +1,201 @@ | |||
Apache License |
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.
Should the top-level LICENSE file be updated to ensure people are aware of the mixed licensing that applies to the repo?
Semi-related: Should we reach out to Nick and Tom to check if relicensing to BSD is in the cards (not for this CL but in the near future)? I assume it's a single-ish author so far which makes it still relatively easy.
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'll reach out before landing. I suspect they changed the main repo license without changing the wasm one.
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 am happy to relicense the rust code as BSD.
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 a lot for chiming in Nick!
07e01d8
to
2e14554
Compare
Yes I copied master branch as is. |
Following agreement from Nick on mozilla#465, align source-map-mappings with source-map license.
2e14554
to
9f6ce5d
Compare
I removed the license files from wasm folder, updated various files to mention BSD license, and updated github repo reference to mozilla's source-map one. |
Following agreement from Nick on mozilla#465, align source-map-mappings with source-map license.
9f6ce5d
to
a0155da
Compare
CONTRIBUTING.md
Outdated
@@ -152,11 +152,11 @@ $ rustup toolchain install nightly | |||
$ rustup target add wasm32-unknown-unknown --toolchain nightly | |||
``` | |||
|
|||
Next, clone the Rust source used to create `lib/mappings.wasm`: | |||
Move to wasm sources folder: | |||
|
|||
``` | |||
$ git clone https://github.com/fitzgen/source-map-mappings.git |
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.
Should this line be removed now that we have the mappings in this repo?
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.
yes, forgot to remove the command line!
wasm-mappings/CONTRIBUTING.md
Outdated
# Contributing to `source-map-mappings` | ||
|
||
Hi! We'd love to have your contributions! If you want help or mentorship, reach | ||
out to us in a GitHub issue, or ping `fitzgen` in [#rust on irc.mozilla.org](irc://irc.mozilla.org#rust) |
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.
Is this irc channel still valid?
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.
irc server is down, I removed it with the new matrix channel.
@@ -0,0 +1,33 @@ | |||
# `source-map-mappings` |
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.
Sould this become wasm-mappings
?
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.
No, the rust package name is still source-map-mappings (you can see it referenced in cargo files)
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.
LGTM! Just had comments/questions about documentation updates
Thanks
Following agreement from Nick on mozilla#465, align source-map-mappings with source-map license.
a0155da
to
066357e
Compare
I've also updated the patch to ensure using latest master revision of source-map-mappings. |
Following agreement from Nick on mozilla#465, align source-map-mappings with source-map license.
066357e
to
22bae06
Compare
With the latest changeset refreshing the wasm binary with latest version of rust, there seem to be a significant performance improvement: |
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.
Looks good, provided that the contribution guidelines and the README link to them is fixed.
@@ -0,0 +1,87 @@ | |||
# Contributing to `source-map-mappings` |
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.
The contents of this file should be merged into the root CONTRIBUTING.md
. It's not really tenable to e.g. apply a separate CoC to a part of a repo as this implies.
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 dropped the redundant content, but kept it for wasm specifics instructions.
I may consider moving "updating mappings.wasm" here?
I find it nice to keep the rust/wasm part of the codebase still contained in this subfolder.
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.
Yeah, that makes sense.
Following agreement from Nick on mozilla#465, align source-map-mappings with source-map license.
5cb1a10
to
272d713
Compare
272d713
to
9ad8fa3
Compare
The WASM sources used to be hosted on another personal repo: https://github.com/fitzgen/source-map-mappings/ This patch inlines this repo into wasm-mappings folder so that it is easier to contribute to WASM sources and see the impact of modifications made to it. This also clarify what are the sources of the binary wasm file (lib/mappings.wasm). I picked current master branch revision: 197660faf3dd1167c643797a5ed62cb687482c54. This isn't a straight merge of the original repo changeset. License has been changed to BSD3, readme template has been dropped, documentation has been updated and references to source-map-mappings have been revised.
Following agreement from Nick on mozilla#465, align source-map-mappings with source-map license.
The last generated wasm file was built in 2018 and it is very challenging to rebuild it the exact same. Rebuild it using latest rust binary and libraries: cargo 1.64.0 (387270bc7 2022-09-16) rustc 1.64.0 (a55dd71d5 2022-09-19) rustup 1.25.1 (bb60b1e89 2022-07-12)
9ad8fa3
to
b56a5c7
Compare
I had to request the enabling of Now everything is green and good to go! |
Today, the rust sources used to generate lib/mappings.wasm are hosted on:
https://github.com/fitzgen/source-map-mappings/
This looks unofficial, whereas that's where the sources come from and last binary was built from that repo.
It would be easier to fold the rust sources in this repo to ease contributions to it, while increasing visibility to the rust sources.
While we are at landing sources in-tree, it would also be nice to refresh the wasm binary by building it with latest rust version.
The current binary has been being in 2018 and is no longer reproducible.
In an effort to help reproduce the same wasm file, here is the environment from which I built it:
Using instruction documented here.