Closed Bug 1154222 Opened 9 years ago Closed 7 years ago

comm-central tryserver does not clean mozilla patches if m-c tip has not changed

Categories

(Release Engineering :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Fallen, Assigned: tomprince)

Details

(Whiteboard: [thunderbird][try])

Attachments

(1 file)

STR:

1. Push a revision to try-comm-central that has --apply-patches in the client.py args
2. Repeat the push before m-c tip changes


Result:
The repository is not cleaned, the old patch is still applied and therefore the build fails.

Expected:
The repository is cleaned every time a try run is executed.

Fixing this could likely be done within client.py. Calling hg purge and hg update -C will help, but it should be made sure that this is only done when running via automation, otherwise it might unintentionally purge a developer's local changes.

What about adding an --automation flag which is passed by build automation? It would call hg purge, hg update -C and also automatically applies patches.

This would remove the need for --apply-patches. There is a note on the wiki about removing --hgtool when using --apply-patches "because I'm not convinced if applying a patch affects the hg share or not". It would be nice if we could clarify this while we are at it. Reading http://mercurial.selenic.com/wiki/ShareExtension it sounds to me like there will be no issue, because applying a patch does not commit and hence not change .hg/store
Whiteboard: [thunderbird][try]
I have been having issues with C-C TB tryserver.

I can't seem to change files under M-C tree portion of the tree under C-C.
Can this bug be related to my problem?

Without "--apply-patches", the M-C portion of the files were NOT touched and
I could proceed to compile and build files.
However, obviously this is not ideal since I need to reference my local changes of M-C files.

With "--apply-patches" I have seen errors as in the following and cannot proceed and build the binary.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=45abe0e0acef

abort: patch failed to apply
subprocess.CalledProcessError: Command '['hg', 'import', '-R',
'mozilla', '-m', 'local patch from
/builds/slave/tb-try-c-cen-lx-00000000000000/build/mozilla-M-C-negative-error-code-processing.patch',
'--no-commit', '--force',
'/builds/slave/tb-try-c-cen-lx-00000000000000/build/mozilla-M-C-negative-error-code-processing.patch']'
returned non-zero exit status 255
make: *** [run_client_py] Error 1

I wonder how I can put the status of M-C portion of the tree in my tryserver job back to normal (or the latest (?)) so that I can proceed.

Any tips will be appreciated.

I was checking other people's jobs on tryserver and several jobs that I checked did not have 
patches for M-C portion of the tree and thus did not seem to have the issue.

TIA
Attached patch Workaround - v1Splinter Review
I used this hack to clean up the m-c patches. Hope it still works!
(In reply to Philipp Kewisch [:Fallen] from comment #2)
> Created attachment 8623297 [details] [diff] [review]
> Workaround - v1
> 
> I used this hack to clean up the m-c patches. Hope it still works!

Thank you very much.
This would help me in the future when I got stuck.
Also, now I realize that I can change the environment of compilation by changing client.py, etc.

*HOWEVER*, after noticing my problem seems to persist even after M-C tip has changed,
I analyzed the failure log very carefully and realized that my problem is related to the following bug/feature.

Bug 1175398 - comm-central tryserver applies M-C patches in ALPHABETICAL ORDER and not in the order in the patch queue

Back in 2013 and earlier when I used Tryserver, either 
my M-C patches had filenames that HAPPENED to be in the right sorting order (or that the patches are not interdependent.)!

Hard to believe, but I have not changed my job submission script since then. So either I was extremely lucky  and/or there has been a subtle change on the server.

TIA
(In reply to ISHIKAWA, Chiaki from comment #3)

> Hard to believe, but I have not changed my job submission script since then.
> So either I was extremely lucky  and/or there has been a subtle change on
> the server.

I was extremely lucky in that my patches are mutually independent (there was no temporal dependency before.
So I modified my job preparation script to create M-C patches in the desired order by clever naming of M-C patch files (so that alphanumerical order is the applied order of the patches). All is well except for frequent failure of Windows build.
But that is another matter.

Thank you again for the tips.
(In reply to Jorg K (GMT+1) from comment #5)

> FWIW: I updated
> https://wiki.mozilla.org/ReleaseEngineering/
> ThunderbirdTryServer#Pushing_mozilla-central_patches

There is this sentence.

>(Note: This is outdated, as of November 2015 you need to replace --hgtool=../tools/buildfarm/utils/hgtool.py and --hgtool1=../scripts/buildfarm/utils/hgtool.py). 

The latter option name is "hgtool*1*". Is this correct? That is do we have to have a digit "1" at the end of option name?

Also, I am not sure what it is meant by
"replacing ...": with what?



TIA
Replace both:

diff --git a/build/client.py-args b/build/client.py-args
--- a/build/client.py-args
+++ b/build/client.py-args
@@ -1,1 +1,1 @@
---hg-options='--time' --hgtool=../tools/buildfarm/utils/hgtool.py --hgtool1=../scripts/buildfarm/utils/hgtool.py --skip-chatzilla --skip-comm --skip-inspector --tinderbox-print
+--hg-options='--time' --apply-patches --skip-chatzilla --skip-comm --skip-inspector --tinderbox-print
(In reply to ISHIKAWA, Chiaki from comment #6)
> There is this sentence.
> 
> >(Note: This is outdated, as of November 2015 you need to replace --hgtool=../tools/buildfarm/utils/hgtool.py and --hgtool1=../scripts/buildfarm/utils/hgtool.py). 
> 
> The latter option name is "hgtool*1*". Is this correct? That is do we have
> to have a digit "1" at the end of option name?

Yes. I have updated the example script with this and made the old state be in the Note, not the current one :)

> Also, I am not sure what it is meant by
> "replacing ...": with what?

It is meant to just remove those options.

I have updated the wiki page as I have now successfully pushed a batch containing both c-c and m-c patches, thanks to these instructions. https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ec896cdee6739854c7cc753e268e9998f8f68b4d
Philipp, is your patch a hack or a solution? Why don't we land it permanently?
Flags: needinfo?(philipp)
Flags: needinfo?(aleth)
As mentioned in comment 2, it is a hack. Doing hg revert --all when running client.py locally does not seem like a good idea.
Flags: needinfo?(philipp)
Any idea for a real fix?
Make it so that the revert only happens on try builds. You are welcome to give it a go.
I have no ambition to dig around with python and the build system. I believe in "biggest bang for the buck", meaning that the best-suited person does the job. BTW: You started bug 1248971 in Feb. 2016 complaining about editing problems in paragraph mode. Guess what: The problem you complained about and many more got fixed so you can enjoy worry-free paragraph editing.
Flags: needinfo?(aleth)
perhaps something for tom
Flags: needinfo?(mozilla)
Some testing indicates that applying patches works find with hgtool, which also makes sure to clean up the working directory of any uncommitted changes. So we should just change the instructions on the wiki to skip removing those options. (I've requested a wiki account to make the change).

We could also just unconditionally add --apply-patches to build/client.py-args although I'm not sure if we want to be doing that for non-try builds. Maybe if we added a commit hook to c-c/c-b etc to ensure that no .patch files are committed?
Assignee: nobody → mozilla
Flags: needinfo?(mozilla)
I'm going to close this, as the new instructions on https://wiki.mozilla.org/ReleaseEngineering/ThunderbirdTryServer#Pushing_mozilla-central_patches correctly clean up old patches. Somebody can open a new bug to discuss supplying --apply-patches unconditionally, but I probably won't look at that until after the taskcluster migration, when things may look differently anyway.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: