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

Moved to a workspace architecture #247

Merged
merged 7 commits into from
Feb 14, 2020
Merged

Moved to a workspace architecture #247

merged 7 commits into from
Feb 14, 2020

Conversation

Razican
Copy link
Member

@Razican Razican commented Feb 5, 2020

This fixes #239, and superseeds #241.

I also took the opportunity to generate the Cargo.lock file with the new Rust 1.41+ format, and add a couple of build profiles. Everything seems to be working fine, except for the wasm feature.

When building it like that, I seem to be getting a ./pkg/ directory, with an empty index.js file. This ./pkg/ directory seems not to be ignored by gitignore. Do you know how this works? I'm not familiarized with Rust-wasm setups.

This fixes #239, and superseeds #241.
@adumbidiot
Copy link
Contributor

Was this PR supposed to address #140 too?

@adumbidiot
Copy link
Contributor

Looks like wasm-pack doesn't support workspaces.

@adumbidiot
Copy link
Contributor

adumbidiot commented Feb 6, 2020

I take that back. We can manually specify the crate to wasmify in the webpack plugin config and change the outdir. Reference: https://github.com/wasm-tool/wasm-pack-plugin.

The pkg dir was empty before because wasm-pack failed.

@Razican
Copy link
Member Author

Razican commented Feb 6, 2020

Was this PR supposed to address #140 too?

Actually, not, but let's do that too!

I take that back. We can manually specify the crate to wasmify in the webpack plugin config and change the outdir. Reference: wasm-tool/wasm-pack-plugin.

The pkg dir was empty before because wasm-pack failed.

I will check that asap and update the PR :) thanks for the pointer!

@Razican
Copy link
Member Author

Razican commented Feb 7, 2020

The build error is due to the binary being run in the benchmarks by default (where criterion is not available). This can be solved by passing -p Boa to cargo bench, done in the criterion compare action (https://github.com/jasonwilliams/criterion-compare-action/blob/master/main.js#L14-L18).

This should probably be added to the benchmarks GitHub action too, and it should be generic enough to accept an input "package" parameter.

@Razican Razican marked this pull request as ready for review February 7, 2020 12:59
@Razican Razican requested a review from jasonwilliams February 7, 2020 12:59
@jasonwilliams
Copy link
Member

jasonwilliams commented Feb 10, 2020

Seems ok from what i can see.
Done https://github.com/jasonwilliams/criterion-compare-action/blob/master/main.js

cargo bench -p Boa -- --save-baseline changes doesn't seem to be working :/

@Razican
Copy link
Member Author

Razican commented Feb 11, 2020

Checking at the output of the action run, it seems that the change is not being run, it's still running cargo bench -- --save-baseline changes :/

It's like it had some cache for the action. I'm guessing that versioning it should help. I will add the commit hash, to check it just in case.

@Razican
Copy link
Member Author

Razican commented Feb 11, 2020

It seems it doesn't work. It's using the minified version in dist/, so that needs to be re-generated.

@jasonwilliams
Copy link
Member

That makes sense, I’ll try it again when I have time

jasonwilliams referenced this pull request in boa-dev/criterion-compare-action Feb 12, 2020
@jasonwilliams
Copy link
Member

built and updated action

@jasonwilliams
Copy link
Member

seems to be weirdly cached, im going to try referencing the commit

@jasonwilliams
Copy link
Member

Looks like we've finally fixed it @Razican

@Razican
Copy link
Member Author

Razican commented Feb 13, 2020

Cool! we can merge then. Still, it's weird it gets cached. We can maybe try switching to the master branch in the future :)

Copy link
Member

@jasonwilliams jasonwilliams left a comment

Choose a reason for hiding this comment

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

Ok lets do this, it looks fine to me

@jasonwilliams jasonwilliams merged commit 5f6e4c2 into boa-dev:master Feb 14, 2020
@Razican Razican deleted the workspace branch February 24, 2020 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make "structopt" dependency optional
3 participants