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

Deprecate util/dev in favor of cargo alias #5109

Merged
merged 4 commits into from Jan 31, 2020
Merged

Conversation

@phansch
Copy link
Member

phansch commented Jan 30, 2020

This means one less shell script and a bit more cross-platform support
for contributors.

If you've been using ./util/dev before, this now becomes cargo dev.

The key part of this change is found in .cargo/config where an alias for calling the clippy_dev binary is defined.

changelog: none

@phansch

This comment has been minimized.

Copy link
Member Author

phansch commented Jan 30, 2020

Not sure if this will pass CI already and it will conflict with #5088 for now.

Also not sure about what this part in ./util/dev does, it may just be for CI?

CARGO_TARGET_DIR=$(pwd)/target/
export CARGO_TARGET_DIR
@flip1995

This comment has been minimized.

Copy link
Member

flip1995 commented Jan 30, 2020

Don't worry about #5088, it's an easy change there

The env var makes sure, that clippy_dev gets build in the same target dir as clippy itself. This way libraries can be shared between clippy and clippy_dev. I think there is a cargo flag to also set this.

The env var also gets set by CI

@@ -1,5 +1,6 @@
[alias]
uitest = "test --test compile-test"
dev = "run --package clippy_dev --bin clippy_dev --manifest-path clippy_dev/Cargo.toml --"

This comment has been minimized.

Copy link
@lzutao

lzutao Jan 30, 2020

Contributor

Use --target-dir flag I believe

@phansch

This comment has been minimized.

Copy link
Member Author

phansch commented Jan 30, 2020

Regarding the CARGO_TARGET_DIR value, I believe we don't need to set it anymore. What util/dev did, was setting it to be the same as the root project's target dir before cding into clippy_dev. When running cargo dev foo from the root project, it's already correct because we don't cd anymore.

@phansch phansch force-pushed the phansch:ciao_util_dev branch from f9141d4 to 3f914c5 Jan 30, 2020
phansch added 2 commits Jan 30, 2020
This allows us to use the method in both `fmt.rs` and `lib.rs` in
multiple places. The downside is that we panic inside the method now,
instead of using the error handling in `fmt.rs`. We may want to
centralize the error handling for clippy_dev at some point, though.
If you've been using `./util/dev` before, this now becomes `cargo dev`.

The key part of this change is found in `.cargo/config`.

This means one less shell script and a bit more cross-platform support
for contributors.
@phansch phansch force-pushed the phansch:ciao_util_dev branch from 848ce7a to 3036a2c Jan 30, 2020
@phansch phansch changed the title WIP: Deprecate util/dev in favor of cargo alias Deprecate util/dev in favor of cargo alias Jan 30, 2020
@phansch

This comment has been minimized.

Copy link
Member Author

phansch commented Jan 30, 2020

GitHub seems to show the wrong commit order: 3036a2c is the second commit and contains some more cleanup refactoring for finding the project root directory.

The PR should be ready for review now

Copy link
Member

flip1995 left a comment

r=me with last occurrence of copy of project_root() function removed.

clippy_dev/src/fmt.rs Show resolved Hide resolved
phansch and others added 2 commits Jan 31, 2020
Co-Authored-By: Philipp Krones <hello@philkrones.com>
@phansch

This comment has been minimized.

Copy link
Member Author

phansch commented Jan 31, 2020

@bors r=flip1995

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 31, 2020

📌 Commit 8f457fa has been approved by flip1995

bors added a commit that referenced this pull request Jan 31, 2020
Deprecate util/dev in favor of cargo alias

This means one less shell script and a bit more cross-platform support
for contributors.

If you've been using `./util/dev` before, this now becomes `cargo dev`.

The key part of this change is found in `.cargo/config` where an alias for calling the `clippy_dev` binary is defined.

changelog: none
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 31, 2020

⌛️ Testing commit 8f457fa with merge ca2abaf...

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 31, 2020

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing ca2abaf to master...

@bors bors merged commit 8f457fa into rust-lang:master Jan 31, 2020
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
@bors bors mentioned this pull request Jan 31, 2020
11 of 11 tasks complete
@phansch phansch deleted the phansch:ciao_util_dev branch Jan 31, 2020
@lzutao

This comment has been minimized.

Copy link
Contributor

lzutao commented Jan 31, 2020

When running cargo dev foo from the root project, it's already correct because we don't cd anymore

No, it is not. When unset CARGO_TARGET_DIR env, running cargo dev fmt will run clippy_dev/target/debug/clippy_dev, not target/debug/clippy_dev.

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

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.