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 crate_universe into it's own directory #651

Merged
merged 5 commits into from
Mar 29, 2021
Merged

Moved crate_universe into it's own directory #651

merged 5 commits into from
Mar 29, 2021

Conversation

UebelAndre
Copy link
Collaborator

Given that there is support for interfacing with this rule without any Cargo.toml files, I would say it's not a "cargo" tool and should go elsewhere. It may still use cargo under the hood but I don't think this is relevant enough to warrant this rule living in ./cargo.

This PR moves the crate_universe rule into it's own directory and workspace.bzl has been renamed to defs.bzl to be consistent with ./rust

@google-cla google-cla bot added the cla: yes label Mar 29, 2021
@UebelAndre
Copy link
Collaborator Author

cc @illicitonion

Copy link
Collaborator

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

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

Works for me!

integration.rs will need a couple of changes to some hard-coded strings, too

crate_universe/CONTRIBUTING.md Outdated Show resolved Hide resolved
crate_universe/CONTRIBUTING.md Outdated Show resolved Hide resolved
UebelAndre and others added 3 commits March 29, 2021 07:06
Co-authored-by: Daniel Wagner-Hall <dawagner@gmail.com>
Co-authored-by: Daniel Wagner-Hall <dawagner@gmail.com>
@UebelAndre
Copy link
Collaborator Author

@illicitonion I cannot find what you're referring to in the test. They feel quite unmaintainable IMO and would opt to silence it for now. I feel the very next thing that should be done is to update the output to be a well formed, pre-rendered, json file or something. #653

@illicitonion
Copy link
Collaborator

I cannot find what you're referring to in the test.
Oh, yeah, I completely lied, no change needed :)

They feel quite unmaintainable IMO and would opt to silence it for now. I feel the very next thing that should be done is to update the output to be a well formed, pre-rendered, json file or something. #653

Yeah, I agree :) I'm about to send out a PR with some smaller, much more targeted tests (though they don't fully replace the integration tests yet). FWIW the reason that the existing test is failing is because someone released a new version of which, and the test uses floating dependencies, which we shouldn't do! I'll fix it up in a follow-up :)

@illicitonion illicitonion merged commit e744b93 into bazelbuild:main Mar 29, 2021
@UebelAndre
Copy link
Collaborator Author

FWIW the reason that the existing test is failing is because someone released a new version of which, and the test uses floating dependencies, which we shouldn't do! I'll fix it up in a follow-up :)

#654 😄

@UebelAndre UebelAndre deleted the not-cargo branch March 29, 2021 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants