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

[Merged by Bors] - Switch to workspace inherited properties #2297

Closed
wants to merge 3 commits into from

Conversation

jedel1043
Copy link
Member

@jedel1043 jedel1043 commented Sep 23, 2022

This Pull Request switches our codebase to the brand new workspace inherited keys, which allows us to define common package options that are usable within each crate's Cargo.toml file.

It also allows to share dependency versions between crates, but I defined only shared versions for our workspace members. It would be a good follow-up to lift all the shared dependencies between crates into the global Cargo.toml.

@jedel1043 jedel1043 added dependencies Pull requests that update a dependency file rust Pull requests that update Rust code Internal Category for changelog labels Sep 23, 2022
@jedel1043 jedel1043 added this to the v0.17.0 milestone Sep 23, 2022
@github-actions
Copy link

github-actions bot commented Sep 23, 2022

Test262 conformance changes

VM implementation

Test result main count PR count difference
Total 92,057 92,057 0
Passed 68,622 68,622 0
Ignored 16,832 16,832 0
Failed 6,603 6,603 0
Panics 0 0 0
Conformance 74.54% 74.54% 0.00%

@codecov
Copy link

codecov bot commented Sep 23, 2022

Codecov Report

Merging #2297 (07470d8) into main (319a907) will decrease coverage by 0.01%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main    #2297      +/-   ##
==========================================
- Coverage   41.25%   41.24%   -0.02%     
==========================================
  Files         237      237              
  Lines       22345    22347       +2     
==========================================
- Hits         9219     9216       -3     
- Misses      13126    13131       +5     
Impacted Files Coverage Δ
boa_engine/src/object/mod.rs 20.55% <50.00%> (ø)
boa_engine/src/syntax/lexer/private_identifier.rs 25.00% <0.00%> (-3.58%) ⬇️
boa_engine/src/syntax/ast/node/template/mod.rs 51.72% <0.00%> (-3.45%) ⬇️
...a_engine/src/syntax/ast/node/statement_list/mod.rs 78.12% <0.00%> (-3.13%) ⬇️
boa_engine/src/syntax/lexer/comment.rs 53.84% <0.00%> (-2.92%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Cargo.toml Outdated Show resolved Hide resolved
@jedel1043
Copy link
Member Author

jedel1043 commented Sep 23, 2022

Apparently dependabot doesn't have support for inherited dependencies (tracked in dependabot/dependabot-core#5315), so lifting the common dependencies is blocked by that issue. Doesn't affect this PR though, since this only lifts intra-workspace dependencies

Copy link
Member

@raskad raskad left a comment

Choose a reason for hiding this comment

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

Looks good to me.
We should probably keep this change in mind for the 0.17 release, to see if it works correct with cargo-workspaces publish.

Cargo.toml Outdated

[workspace.dependencies]
boa_engine = { version = "0.15.0", path = "boa_engine" }
boa_interner = { version = "0.15.0", path = "boa_interner" }
Copy link
Member

Choose a reason for hiding this comment

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

What happens if version is not specified?

Copy link
Member Author

Choose a reason for hiding this comment

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

Crates.io rejects publishing the crate

@jedel1043
Copy link
Member Author

jedel1043 commented Sep 23, 2022

Should we push this into 0.16, just to see if cargo-workspaces work? We can rollback if there's any problem

Edit: Apparently there are still some issues to be solved. We should delay this to 0.17
rust-lang/cargo#11133
pksunkara/cargo-workspaces#71
killercup/cargo-edit#752

@raskad
Copy link
Member

raskad commented Sep 25, 2022

bors r+

bors bot pushed a commit that referenced this pull request Sep 25, 2022
This Pull Request switches our codebase to the brand new [workspace inherited keys](https://doc.rust-lang.org/cargo/reference/workspaces.html#the-package-table), which allows us to define common package options that are usable within each crate's Cargo.toml file.

It also allows to share dependency versions between crates, but I defined only shared versions for our workspace members. It would be a good follow-up to lift all the shared dependencies between crates into the global Cargo.toml.
@bors
Copy link

bors bot commented Sep 25, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Switch to workspace inherited properties [Merged by Bors] - Switch to workspace inherited properties Sep 25, 2022
@bors bors bot closed this Sep 25, 2022
@bors bors bot deleted the new_workspaces branch September 25, 2022 20:24
@poliorcetics
Copy link

You won't have any rust-related dependency update from dependabot now I think since it doesn't yet know where to find the version number for a dep.workspace = true dependency, even if that dependency is local.

I opened a PR for it dependabot/dependabot-core#5794, it should work but it's not merged yet.

It's probably possible to regenerate the docker container and use it locally to make the necessary changes but I have no idea how to do that. I'll gladly accept people testing it and telling me if it works has expected in a variety of configs though 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file Internal Category for changelog rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants