1546925 - Allow preceding text in try comment

Status: RESOLVED FIXED

(NSS :: Build, defect)

Description

[Pete Moore [:pmoore][:pete]]

Bug 1525946 introduced a bug in the try syntax parsing.

Previously, the try commit syntax allowed text to appear before the try syntax, e.g. "Bug 12345 - fix some issue; try: -p win64 -u all -t none".

However, since bug 1525946 landed, it is no longer possible to have preceding text, i.e. you would need to commit with "try -p win64 -u all -t none".

The reason for this is that prior to that bug, mozilla-taskcluster was stripping out the try comment here. It was then referenced in .taskcluster.yml to place it in the environment variable TC_COMMENT. Later on this is matched against a regular expression here (credits to Martin Thomson for spotting this) which allows only whitespace padding from the start of the line.

With bug 1525946, the raw hg commit comment is passed through to the regular expression, and therefore fails to match, thereby causing all tasks to get executed.

Comment 1

[Pete Moore [:pmoore][:pete]]

Attachment: bug1546925_nss_v1.patch

The regular expression I went for is to allow:

• any text preceding 'try:' as long as the last character is not [a-zA-Z]
• no text preceding 'try:'

This is in order to match e.g.

• try: ...
• Bug 12345 - lorem ipsum;try: ...

But to not match:

• Psychiatry: A Clinical Treatise On Diseases of the Fore-Brain Based Upon a Study of Its Structure, Functions, and Nutrition by Theodor Meynert

Comment 2

[Pete Moore [:pmoore][:pete]]

Unfortunately the try push doesn't seem to be running...

Comment 3

[Pete Moore [:pmoore][:pete]]

Attachment: bug1546925_nss_v2.patch

I've improved the commit comment, and made the regular expression a multiline regular expression (/m at end), since we used to allow this too here.

Comment 4

[Pete Moore [:pmoore][:pete]]

Seems to have worked: https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=4fccf44a27404ecc73d9ad986712dda0d3d1d6b1

The commit message was also multiline: https://hg.mozilla.org/projects/nss-try/rev/4fccf44a27404ecc73d9ad986712dda0d3d1d6b1

Comment 5

[Pete Moore [:pmoore][:pete]]

Attachment: bug1546925_nss_v3.patch

Applied simplification (and improvement!) suggested by :mt in IRC.

Comment 6

[Pete Moore [:pmoore][:pete]]

Weird - the v3 try push didn't filter the platforms, but the v2 try push did. They used the same commit comment.

The difference was,

• v2: let match = comment.match(/^(?:.*[^a-zA-Z])?try:\s*(.*)\s*$/m);
• v3: let match = comment.match(/^\btry:\s*(.*)\s*$/m);

The commit comment used in both cases was:

Bug 1546925 - allow arbitrary leading text before try syntax,try: -p win64 -u none -t none

Let's see how this goes...

Should we go back to v2 version, or shall we try to find out why v3 version failed?

Comment 7

[Pete Moore [:pmoore][:pete]]

Ahhhhh - I should have removed the ^ !!

Comment 8

[Pete Moore [:pmoore][:pete]]

Attachment: bug1546925_nss_v4.patch

Fourth time lucky?! :D

Comment 9

[Pete Moore [:pmoore][:pete]]

Success!

https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=68d647fee7f56cc771a9385644e96bf6efff3de0

Comment 10

[Pete Moore [:pmoore][:pete]]

(In reply to Pete Moore [:pmoore][:pete] from comment #2)

Unfortunately the try push doesn't seem to be running...

Seems to be a problem either with the hook definition, or the hooks service. Created bug 1546951 for this.

Comment 11

[Martin Thomson [:mt:]]

Comment on attachment 9060695 [details] [diff] [review]
bug1546925_nss_v4.patch

I'll have to ask you to use Phabricator in future.

https://hg.mozilla.org/projects/nss/rev/3895acc65f37ed3c18ad9b23837aa4999d62c150