1019955 - "Start server" getting into a self-hosted file?

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=40983644&tree=Mozilla-Inbound&full=1 contains the error message

Executing /builds/slave/m-in-lx-0000000000000000000000/build/obj-firefox/dist/bin/xpcshell -g /builds/slave/m-in-lx-0000000000000000000000/build/obj-firefox/dist/bin/ -a /builds/slave/m-in-lx-0000000000000000000000/build/obj-firefox/dist/bin/ -f /builds/slave/m-in-lx-0000000000000000000000/build/toolkit/mozapps/installer/precompile_cache.js -e precompile_startupcache("resource://gre/");
self-hosted:1:6 SyntaxError: missing ; before statement:
self-hosted:1:6 Start server
self-hosted:1:6 ......^

It seems something is printing "Start server" on its output and that is making it into something that should be a source file.

This has hit a few times in the last week.

Comment 1

[Till Schneidereit [:till]]

I've seen this before about 10 days ago.

Changeset 91eb27108c0b was backed out for it, but the same thing happened again on the backout: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=11090a948663

Relanding with only a wholly-unrelated change before worked just fine: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=315b244f4e90

I don't know the details, but according to Tomcat this was infrastructure-related. Carsten, do you know more about what went on there?

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Highly doubt this is infra. It's specifically tied to patches that touch self-hosting.

Comment 3

[Till Schneidereit [:till]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #2)
> Highly doubt this is infra. It's specifically tied to patches that touch
> self-hosting.

I do see that there is a correlation there, but I don't understand it. Why would it be tied to a self-hosting-touching patch, then not go away upon backout, but not exist on the next landing of said patch? How would the first landing of the patch be any different from subsequent builds with the change included?

Comment 4

[Ryan VanderMeulen [:RyanVM]]

We go to a lot of lengths to recycle builders as much as possible in order to minimize build times (by reusing an already-current objdir). So in fact, it makes perfect sense. The underlying issue is there regardless of the patch that manages to tickle it. So both the original landing and the backout can tickle the same problem. The only sure-fire way to make it go away is to clobber and clear that bad-state objdir and force the self-hosted stuff to rebuild.

Comment 5

[Carsten Book [:Tomcat]]

yeah in the case till mentioned it was i think the mix case of clobber and clear the bad state possible. When i tried to check the slave that failed on backout and original checkin the slave went away (was not reachable and so a new slave finally took over on retrigger - so maybe this was the case with kind of clobber here :)

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Happening again on inbound thanks to bug 934423.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2184d492b374

https://tbpl.mozilla.org/php/getParsedLog.php?id=41034875&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=41034885&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=41035465&tree=Mozilla-Inbound

Comment 7

[Ryan VanderMeulen [:RyanVM]]

This needs attention ASAP as it's a cause of frequent, unpredictable bustage.

Comment 8

[Jeff Walden [:Waldo]]

Is it truly unpredictable?  Without fully understanding the issue, but observing the self-hosting connection, I would expect this to trigger for changes to anything listed in selfhosted_out_h_deps in js/src/Makefile.in.  That list is not small.  But it's not all that large, either -- particularly if you consider the js/src/builtin/*.js files as basically one conceptual category.  For now I would exercise preemptive CLOBBER for anything that touches those files, while still keeping priority bumped here.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

How are you planning to effectively communicate that to *all* JS devs?

Comment 10

[Jeff Walden [:Waldo]]

Constant vigilance, reviewer diligence.  It's not a large set of people who do reviews, for the most part.  We can post to js-internals and pick up most people that way, I expect.

Comment 11

[Jeff Walden [:Waldo]]

Sent mail to js-internals newsgroup/list about this.  And I'll poke people on IRC when I head to the office shortly, too.

Comment 12

[Ed Morley [:emorley]]

This is believed responsible for the currently mass B2G desktop failures on b2g-inbound, which is holding the tree closed until clobbered build retriggers (and the associated subsequent tests) come back green - which will be several hours.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ca77e129db83

Comment 14

[Shu-yu Guo [:shu]]

I'm pretty sure this is because of sccache: https://github.com/glandium/sccache/blob/master/sccache.py#L51

What embedjs.py (the script that processes all the self-hosted files) does is take a path to cpp, the C preprocessor, and saves the stdout that cpp gives back.

The path to cpp on TBPL looks like: 'python2.7 /builds/slave/m-in-l64-000000000000000000000/build/sccache/sccache.py /builds/slave/m-in-l64-000000000000000000000/build/gcc/bin/gcc -E'

Every once in a while, sccache.py prints out 'Start server' to stdout, which gets slurped up by embedjs.py

The easiest fix would make sccache.py print 'Start server' on stderr or something. I don't remember why embedjs.py pipes cpp's stdout instead of making stdout write to a temporary file. I think perhaps because I couldn't find a flag for it for MSVC or something?

Comment 15

[Shu-yu Guo [:shu]]

s/making stdout write to a temporary file/making cpp write to a temporary file

Comment 16

[Till Schneidereit [:till]]

Attachment: Write preprocessing results for self-hosted code into file instead of stdout

I think this should do the trick. Try-servering here:
https://tbpl.mozilla.org/?tree=Try&rev=674c4d7149fc

Should also fix bug 980793.

Comment 17

[Mike Hommey [:glandium] OoO until Dec 30]

Comment on attachment 8435418 [details] [diff] [review]
Write preprocessing results for self-hosted code into file instead of stdout

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

::: js/src/builtin/embedjs.py
@@ +86,2 @@
>      output.write('\n'.join([msgs] + ['#include "%(s)s"' % { 's': source } for source in sources]))
> +  cmdline = cxx + preprocessorOption + [processedSources] +\

I don't expect this to work on Windows: the command line argument is -Feoutputfile, not -Fe outputfile.

@@ +132,5 @@
>  #include "%(msgs)s"
>  """ % { 'msgs': msgs })
> +  p = subprocess.check_output(cxx + preprocessorOption + [processedMsgs] + [msgsTemplate])
> +  with open(processedMsgs, 'r') as input:
> +    processed = input.read()

You should have a function to do preprocessing instead of having two manual invocations of subprocess.check_output.

Comment 18

[Till Schneidereit [:till]]

Attachment: Write preprocessing results for self-hosted code into file instead of stdout. v2

Right you are. This is somewhat refactored and should fare better: https://tbpl.mozilla.org/?tree=Try&rev=4b38349c9e08

Comment 19

[Till Schneidereit [:till]]

Comment on attachment 8435719 [details] [diff] [review]
Write preprocessing results for self-hosted code into file instead of stdout. v2

Ok, the Windows build still doesn't succeed. I don't understand what's going on there, yet, but am looking into it.

Comment 20

[Till Schneidereit [:till]]

Ok, so I really don't understand what's going on here. I thought that maybe the relative paths or something about using check_output instead of Popen is responsible for the broken builds on Windows, but my latest try push doesn't do either of those:
https://tbpl.mozilla.org/?tree=Try&rev=011202b670d5

It works on my Windows machine, and everywhere else, but the build machine complains "WindowsError: [Error 2] The system cannot find the file specified". I don't even know which file is meant or how to debug this.

Glandium, can you take a look at the patch[1] and the log to see if you know what's going on?


[1]: https://hg.mozilla.org/try/rev/4487b82856d0
[2]: https://tbpl.mozilla.org/php/getParsedLog.php?id=41438902&tree=Try&full=1#error4

Comment 21

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8435719 [details] [diff] [review]
Write preprocessing results for self-hosted code into file instead of stdout. v2

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

::: js/src/builtin/embedjs.py
@@ +119,2 @@
>    # Clang seems to complain and not output anything if the extension of the
> +  # input is not something it recognizes, so just fake a .cpp here.

It'd be a little cleaner to pass in |-x c++|

Comment 22

[Till Schneidereit [:till]]

(In reply to Steve Fink [:sfink] from comment #21)
> It'd be a little cleaner to pass in |-x c++|

True, but that doesn't work with VSCC :( There's probably an equivalent, but we'd have to pass the flag to use in and set up the correct build script var in the first place, etc.

Comment 23

[Mike Hommey [:glandium] OoO until Dec 30]

http://bugs.python.org/issue8557

You need to resolve the path of the program before executing it (use which.which, it's in the virtualenv)

Comment 24

[Till Schneidereit [:till]]

Attachment: Write preprocessing results for self-hosted code into file instead of stdout. v3

Thanks again for the help over IRC. Try-servering with which.which now: https://tbpl.mozilla.org/?tree=Try&rev=90ddce49150f

Comment 25

[Mike Hommey [:glandium] OoO until Dec 30]

Comment on attachment 8438235 [details] [diff] [review]
Write preprocessing results for self-hosted code into file instead of stdout. v3

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

::: js/src/builtin/embedjs.py
@@ +90,5 @@
> +  combinedSources = '\n'.join([msgs] + ['#include "%(s)s"' % { 's': source } for source in sources])
> +  args = ['-D%(k)s=%(v)s' % { 'k': k, 'v': env[k] } for k in env]
> +  preprocessed = preprocess(cxx, preprocessorOption, combinedSources, args)
> +  processed = '\n'.join([line for line in preprocessed.splitlines() if not\
> +                         (len(line) == 0 or line.isspace() or line.startswith('#'))])

This condition can be written as if line.strip() and not line.startswith('#') which might be more readable. ymmv.

@@ +121,4 @@
>    # Clang seems to complain and not output anything if the extension of the
> +  # input is not something it recognizes, so just fake a .cpp here.
> +  if (not os.path.exists(cxx[0])):
> +    cxx[0] = which.which(cxx[0])

You may want to move this above the comment so that the comment stays near the code it applies to.

@@ +124,5 @@
> +    cxx[0] = which.which(cxx[0])
> +  tmpIn = 'self-hosting-cpp-input.cpp';
> +  tmpOut = 'self-hosting-preprocessed.pp';
> +  outputArg = shlex.split(preprocessorOption)
> +  outputArg[-1] += tmpOut

outputArg = shlex.split(preprocessorOption + tmpOut) would be more accurate.

@@ +126,5 @@
> +  tmpOut = 'self-hosting-preprocessed.pp';
> +  outputArg = shlex.split(preprocessorOption)
> +  outputArg[-1] += tmpOut
> +
> +  with open(tmpIn, 'wb') as output:

input

@@ +131,5 @@
> +    output.write(source)
> +  print(' '.join(cxx + outputArg + args + [tmpIn]))
> +  result = subprocess.Popen(cxx + outputArg + args + [tmpIn]).wait()
> +  if (result != 0):
> +    exit(result);

sys.exit

@@ +132,5 @@
> +  print(' '.join(cxx + outputArg + args + [tmpIn]))
> +  result = subprocess.Popen(cxx + outputArg + args + [tmpIn]).wait()
> +  if (result != 0):
> +    exit(result);
> +  with open(tmpOut, 'r') as input:

output

Comment 26

[Till Schneidereit [:till]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2e3a1b454899

Thanks for the review!

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2e3a1b454899