Closed Bug 1050601 Opened 6 years ago Closed 6 years ago

Remove fix-linux-stack.pl

Categories

(Testing :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla35

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(1 file, 1 obsolete file)

It was rewritten in Python in bug 914253.
Attached patch Remove fix-linux-stack.pl (obsolete) — Splinter Review
Straightforward. I've left the task of better integration with mochitest for
bug 948718.
Attachment #8482508 - Flags: review?(ted)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Comment on attachment 8482508 [details] [diff] [review]
Remove fix-linux-stack.pl

Review of attachment 8482508 [details] [diff] [review]:
-----------------------------------------------------------------

I'd love to stop copying these scripts to dist/bin as well, but we can deal with that in the followup.

::: build/automation.py.in
@@ +698,2 @@
>          # This method is preferred for developer machines, so we don't have to run "make buildsymbols".
> +        stackFixerProcess = self.Process([self.PERL, os.path.join(utilityPath, "fix_linux_stack.py")],

Uh, PERL isn't going to do the trick here. You want sys.executable.

::: testing/mochitest/runtests.py
@@ +1923,2 @@
>          # This method is preferred for developer machines, so we don't have to run "make buildsymbols".
> +        stackFixerCommand = [self.perl, os.path.join(self.utilityPath, "fix_linux_stack.py")]

likewise here
Attachment #8482508 - Flags: review?(ted) → review+
Thank you for the fast review.

> Uh, PERL isn't going to do the trick here. You want sys.executable.

Yikes! And in the conditional above, too.

Alas, my try run didn't show this up: https://tbpl.mozilla.org/?tree=Try&rev=2a91a9b55f3b. I'll work out how to run this code locally and make sure it's still working there.

Is there a difference between |self.perl| and |self.PERL|?
This new version is sufficiently different to the previous that I'm asking for
another review. New stuff:

- Removed more Perl stuff in automation.py.in. In particular, the
  IS_DEBUG_BUILD case is now just like the Mac case above it (though I don't
  why the Mac case has |and False| in its condition). I tested this by running
  the reftests locally.

- Removed more Perl stuff from mochitest/runtests.py. The isLinux case is now
  just like the fix_stack_using_bpsyms case above it, i.e. it calls
  fixSymbols() directly instead of executing the scripts (and again I don't
  know why Mac isn't handled similarly). With this change I was able to remove
  a bunch of timeout stuff, and I may have inadvertently fixed bug 948718. I
  tested this by running mochitests locally.

- This mochitest change required a small fix to fix_linux_stack.py: when it
  modifies its input, it now only append a newline if the input had a newline
  to begin with. This was necessary to avoid extraneous blank lines in the
  mochitest output. fix_stack_using_bpsyms.py doesn't have this behaviour; I
  don't know why.
Attachment #8483251 - Flags: review?(ted)
Attachment #8482508 - Attachment is obsolete: true
 9 files changed, 24 insertions(+), 314 deletions(-)
(In reply to Nicholas Nethercote [:njn] from comment #3)
> Alas, my try run didn't show this up:
> https://tbpl.mozilla.org/?tree=Try&rev=2a91a9b55f3b. I'll work out how to
> run this code locally and make sure it's still working there.

This code only gets hit in local Linux debug builds, since it relies on having unstripped binaries available. (On the plus side, it's very hard to break TBPL tests by breaking this!)


> Is there a difference between |self.perl| and |self.PERL|?

I think that's just differences between automation.py and the code that was copied into runtests.py.
Comment on attachment 8483251 [details] [diff] [review]
Remove fix-linux-stack.pl

Review of attachment 8483251 [details] [diff] [review]:
-----------------------------------------------------------------

Well, you've gone and fixed bug 948718 as well here. :)
Attachment #8483251 - Flags: review?(ted) → review+
No longer blocks: 948718
Duplicate of this bug: 948718
https://hg.mozilla.org/mozilla-central/rev/7a7204eb7350
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.