keep the root dir clean from debugging #65653

RalfJung · 2019-10-21T08:33:50Z

We landed this before with #63307 but recently in #65630 the IMO bad ignore crept back in.

If you regularly do graphviz-based debugging and you are fine leaving junk in the rustc root dir, please configure your local .git/info/exclude. But most people working on rustc don't work with graphciz all that often (I for once never did), and not everyone likes to have stray generated files in their source dirs.

Also Cc #63373 #53768 @ecstatic-morse @Mark-Simulacrum

rust-highfive · 2019-10-21T08:34:02Z

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

Centril · 2019-10-21T09:56:55Z

I think having these in .gitignore provides for a smoother experience overall. Personally, I'm not someone who uses graphviz (at all) and so I wouldn't add them to my local exclude, at the same time, when these .dot files show up, as they did, it is in the way of me hacking on rustc. It's also noteworthy that the comment at the very top of the file contradicts the comment at the bottom.

RalfJung · 2019-10-21T10:25:39Z

when these .dot files show up, as they did

That was a bug. Adding them to .gitignore means papering over the bug instead of fixing it.

he comment at the very top of the file contradicts the comment at the bottom

Why do you think they contradict? I wrote both of them and I think the one at the top implies the one at the bottom (which I added just as clarification and clearly because the one at the top is not enough).

Centril · 2019-10-21T10:32:13Z

That was a bug. Adding them to .gitignore means papering over the bug instead of fixing it.

I agree with your description but I'm explicitly arguing for the unprincipled solution of using duct tape to alleviate some pain. :)

Why do you think they contradict? I wrote both of them and I think the one at the top implies the one at the bottom (which I added just as clarification and clearly because the one at the top is not enough).

Maybe "contradict" is too strong. The first comment does suggest however that ignoring things generated in the build is something the file should do. The latter suggests something in the other direction.

RalfJung · 2019-10-21T11:17:17Z

The first comment does suggest however that ignoring things generated in the build is something the file should do. The latter suggests something in the other direction.

These files don't get generated by a normal build (if they do, it's a bug). They get generated by manually calling rustc -Zdump-mir or so. If we ignore such files we might as well add /* -- an awful idea.

I agree with your description but I'm explicitly arguing for the unprincipled solution of using duct tape to alleviate some pain. :)

The bug is fixed already so I really don't get it. Usually when we fix a bug we add a test to make sure it doesn't come back; you are now instead proposing to actively work towards not noticing regressions.
Mind if I delete most of src/test/ui, then? That would make development much less painful, making tests pass is hard work after all. </sarcasm>

On a somewhat more serious note, this is kind of comparable with deny(warnings). We also are strict by default here which can be painful, and then people can locally relax that.

In my years of working on Rust, this is the one and only time x.py created junk files and now we have a tidy lint protecting against it. If you are personally so bothered by those files showing up for a few days and really don't want to know if this bug ever comes back, just edit your local config (echo '/*' >> .git/info/exclude).

.gitignore

Mark-Simulacrum · 2019-10-21T11:57:22Z

Ah, I was unaware of the policy we had set to not do this in the general case. I think it makes sense to remove the changes to .gitignore (as this PR does) -- I agree that they're papering over what seems to be (and should be) a non-existent problem in a non-ideal way.

I feel similarly opposed/confused I think by @Centril's comment of "duct tape to alleviate some pain" since to our knowledge, there is no more pain anymore (and it should hopefully not arise, due to the tidy lint).

r=me with nit fixed unless you want to wait for broader consensus

ecstatic-morse · 2019-10-21T15:48:32Z

Sorry for the churn. I was unaware of the previous discussions around this, and I didn't fully grok the reasoning for the comment at the top.

Centril · 2019-10-21T16:04:00Z

@RalfJung I think there's a lot of reductio ad absurdum going on in your comment. Obviously I don't agree with ignoring /* nor with removing things testing functional correctness (e.g. src/test/ui).

The bug is fixed already so I really don't get it. Usually when we fix a bug we add a test to make sure it doesn't come back; you are now instead proposing to actively work towards not noticing regressions.

Since the bug has been fixed I'm fine with removing it from .gitignore. I do however think that e.g. adding other things to .gitignore when they aren't fixed, e.g. as a temporary solution or more medium term is OK because these are neither about functional correctness nor code quality (e.g. as with lints and deny(warnings)).

In my years of working on Rust, this is the one and only time x.py created junk files and now we have a tidy lint protecting against it.

That's not my experience, it has happened much more frequently for me (probably because I kill builds quickly sometimes).

All in all I'm fine with this landing but I wanted to register that ignoring things for a while is useful sometimes.

RalfJung · 2019-10-21T16:06:22Z

Nit is fixed but I'd prefer to clarify the wording if we can, so I did some more editing. @Centril I clarified that only the x.py build is meant by "build" here.

@ecstatic-morse how could we improve the comment to avoid repeating this exercise?
(Also when you edit a file with a comment you don't understand, it'd be helpful to point that out in the PR you submit so the reviewer can help you. My interpretation of your PR was that you didn't see the comment at the top because you edited at the bottom, which is why I added another comment at the bottom.)

RalfJung · 2019-10-21T16:32:30Z

I think there's a lot of reductio ad absurdum going on in your comment.

Not reductio ad absurdsum but consistently applying the standards you seem to have for file system cleanliness to other things. I did not expect you to actually agree with any of the other cases, obviously -- but I wanted to point out that those all are different flavors of the same principle for me.

Because you care about having a clean codebase, I thought you might have some empathy for people that have similarly strong feelings for having a clean file system.

Centril · 2019-10-21T16:37:26Z

Because you care about having a clean codebase, I thought you might have some empathy for people that have similarly strong feelings for having a clean file system.

That's fair enough. I do see the value of a clean file system (but there are benefits on the other side too).

Centril · 2019-10-21T16:38:10Z

@bors r=Mark-Simulacrum,Centril rollup

bors · 2019-10-21T16:38:11Z

📌 Commit ebc9a1a has been approved by Mark-Simulacrum,Centril

ecstatic-morse · 2019-10-21T17:01:10Z

@ecstatic-morse how could we improve the comment to avoid repeating this exercise?
(Also when you edit a file with a comment you don't understand, it'd be helpful to point that out in the PR you submit so the reviewer can help you. My interpretation of your PR was that you didn't see the comment at the top because you edited at the bottom, which is why I added another comment at the bottom.)

I meant more that it wasn't worded very strongly, unlike the new comment. I skimmed it thinking it was a generic description of what .gitignore is for and missed the "only".

@ecstatic-morse

…um,Centril keep the root dir clean from debugging We landed this before with rust-lang#63307 but recently in rust-lang#65630 the IMO bad ignore crept back in. If you regularly do graphviz-based debugging and you are fine leaving junk in the rustc root dir, please configure your local `.git/info/exclude`. But most people working on rustc don't work with graphciz all that often (I for once never did), and not everyone likes to have stray generated files in their source dirs. Also Cc rust-lang#63373 rust-lang#53768 @ecstatic-morse @Mark-Simulacrum

@ecstatic-morse

…um,Centril keep the root dir clean from debugging We landed this before with rust-lang#63307 but recently in rust-lang#65630 the IMO bad ignore crept back in. If you regularly do graphviz-based debugging and you are fine leaving junk in the rustc root dir, please configure your local `.git/info/exclude`. But most people working on rustc don't work with graphciz all that often (I for once never did), and not everyone likes to have stray generated files in their source dirs. Also Cc rust-lang#63373 rust-lang#53768 @ecstatic-morse @Mark-Simulacrum

@ghost

Rollup of 7 pull requests Successful merges: - #62330 (Change untagged_unions to not allow union fields with drop) - #65092 (make is_power_of_two a const function) - #65621 (miri: add write_bytes method to Memory doing bounds-checks and supporting iterators) - #65647 (Remove unnecessary trait bounds and derivations) - #65653 (keep the root dir clean from debugging) - #65660 (Rename `ConstValue::Infer(InferConst::Canonical(..))` to `ConstValue::Bound(..)`) - #65663 (Fix typo from #65214) Failed merges: r? @ghost

keep the root dir clean from debugging

55b787e

rust-highfive assigned nikomatsakis Oct 21, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 21, 2019

Mark-Simulacrum reviewed Oct 21, 2019

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

expand comment

ebc9a1a

RalfJung force-pushed the gitignore branch from 1b9888c to ebc9a1a Compare October 21, 2019 16:05

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 21, 2019

Centril mentioned this pull request Oct 21, 2019

Rollup of 7 pull requests #65670

Closed

Centril mentioned this pull request Oct 21, 2019

Rollup of 7 pull requests #65671

Merged

bors merged commit ebc9a1a into rust-lang:master Oct 22, 2019

RalfJung deleted the gitignore branch October 22, 2019 07:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

keep the root dir clean from debugging #65653

keep the root dir clean from debugging #65653

RalfJung commented Oct 21, 2019

rust-highfive commented Oct 21, 2019

Centril commented Oct 21, 2019

RalfJung commented Oct 21, 2019 •

edited

Loading

Centril commented Oct 21, 2019

RalfJung commented Oct 21, 2019 •

edited

Loading

Mark-Simulacrum commented Oct 21, 2019

ecstatic-morse commented Oct 21, 2019

Centril commented Oct 21, 2019

RalfJung commented Oct 21, 2019 •

edited

Loading

RalfJung commented Oct 21, 2019 •

edited

Loading

Centril commented Oct 21, 2019 •

edited

Loading

Centril commented Oct 21, 2019

bors commented Oct 21, 2019

ecstatic-morse commented Oct 21, 2019 •

edited

Loading

keep the root dir clean from debugging #65653

keep the root dir clean from debugging #65653

Conversation

RalfJung commented Oct 21, 2019

rust-highfive commented Oct 21, 2019

Centril commented Oct 21, 2019

RalfJung commented Oct 21, 2019 • edited Loading

Centril commented Oct 21, 2019

RalfJung commented Oct 21, 2019 • edited Loading

Mark-Simulacrum commented Oct 21, 2019

ecstatic-morse commented Oct 21, 2019

Centril commented Oct 21, 2019

RalfJung commented Oct 21, 2019 • edited Loading

RalfJung commented Oct 21, 2019 • edited Loading

Centril commented Oct 21, 2019 • edited Loading

Centril commented Oct 21, 2019

bors commented Oct 21, 2019

ecstatic-morse commented Oct 21, 2019 • edited Loading

RalfJung commented Oct 21, 2019 •

edited

Loading

RalfJung commented Oct 21, 2019 •

edited

Loading

RalfJung commented Oct 21, 2019 •

edited

Loading

RalfJung commented Oct 21, 2019 •

edited

Loading

Centril commented Oct 21, 2019 •

edited

Loading

ecstatic-morse commented Oct 21, 2019 •

edited

Loading