1406346 - Local changes should not break the capability to install binaries from tc

Status: RESOLVED FIXED

(Firefox Build System :: General, defect, P4)

Description

[Sylvestre Ledru [:Sylvestre]]

$ echo "" >> build/build-clang/build-clang.py 
$ hg commit -m "test" build/build-clang/build-clang.py
$ ./mach static-analysis install
Could not find artifacts for a toolchain build named `linux64-clang-tidy`


Best case scenario, "./mach static-analysis install" will still work
Worst case scenario, we should have an error message like 'your local repository (changeset "foo") differs from the one expected (changeset "bar") or you have some artifact in your directory.'

Comment 1

[Gregory Szorc [:gps]]

The artifact identifiers are based on a hash of file inputs. So if you change an input file...

To make this "just work" (which would violate a constraint that used artifacts are based on hash of inputs), we'd have to read the old versions of files from version control. And to detect local changes, we'd also have to query for "local commits." That is achievable in Mercurial via phases. Git is a bit harder because identifying refs is a bit involved.

Either way, we would need to more tightly integrate VCS into the artifacts code. This is a... sensitive topic.

Anyway, the error message isn't a lie. There is certainly room to improve it though. But it feels low priority.

Comment 2

[Sylvestre Ledru [:Sylvestre]]

I don't think it is a low priority.
For none-specialist of that stuff, this is frustrating. 
Works in a clean tree, as soon as you touch a file, it is not working anymore without any clear reason (it is not obvious that it is based on a hash of the local files).

At least, we should have a better error message.

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1406346 - In case of error, tell to the dev that it might be caused by temporary files

Review commit: https://reviewboard.mozilla.org/r/196604/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/196604/

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8925478 [details]
Bug 1406346 - In case of error, tell to the dev that it might be caused by temporary files

https://reviewboard.mozilla.org/r/196604/#review205014

I don't think this error message is very helpful.

How about something like: "Local commits and other changes in your checkout may cause this error. Try updating to a fresh checkout of mozilla-central to use artifact builds."

Comment 5

[Andi [:andi]]

I wonder if it should be a good idea that in the message to also include the paths where potential issues might occur, or maybe if we extend this further do a:

>>hg status

And with the result of the of the hg command match it against our list of paths that are part of the hash? What do you think about this?

Comment 6

[Mike Hommey [:glandium]]

The user may not be using mercurial, and the changes may well be committed.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8925478 [details]
Bug 1406346 - In case of error, tell to the dev that it might be caused by temporary files

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196604/diff/1-2/

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8925478 [details]
Bug 1406346 - In case of error, tell to the dev that it might be caused by temporary files

https://reviewboard.mozilla.org/r/196604/#review206512

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/82ea4b82a7fa
In case of error, tell to the dev that it might be caused by temporary files r=gps

Comment 10

[Gregory Szorc [:gps]]

There is definitely room to improve the detection. IMO the build system should consider rejecting artifact builds if certain files have been changed locally.

ahal recently added a `base_ref` property to mozversioncontrol. With that, it should be possible to collect the set of changed paths between base and current. And with that, we can start building in some more intelligent smarts here.

Comment 11

[Mike Hommey [:glandium]]

This is *not* about artifact builds.

Comment 12

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/82ea4b82a7fa

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:kmoir, maybe it's time to close this bug?

Comment 14

[Kim Moir [:kmoir] ET]

:sylvestre is this still an issue for you or can the bug be closed?

Comment 15

[Sylvestre Ledru [:Sylvestre]]

This isn't perfect but good enough.