Closed Bug 1497446 Opened 6 years ago Closed 6 years ago

[BinAST] Move to binjs_meta 0.4.3

Categories

(Core :: JavaScript Engine, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(4 files, 7 obsolete files)

13.22 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
27.08 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
3.67 MB, patch
Yoric
: review+
Details | Diff | Splinter Review
46 bytes, text/x-phabricator-request
Details | Review
This may require handling PropertyKey and IdentifierName.
Oh, great:
«
The following files exceed the filesize limit of 102400:

third_party/rust/lalrpop/src/parser/lrgrammar.rs
»

That sounds like a problem that was solved a long time ago.
So, we're currently blocked by https://github.com/sgodwincs/webidl-rs/pull/19, which I opened a few hours ago.
Priority: -- → P3
See Also: → 1497378
Summary: [BinAST] Move to binjs_meta 0.4.1 → [BinAST] Move to binjs_meta 0.4.2
Per discussion with Yoric I moved this bug to a P1
Priority: P3 → P1
Assignee: nobody → dteller
Summary: [BinAST] Move to binjs_meta 0.4.2 → [BinAST] Move to binjs_meta 0.4.3
Since binjs_meta 0.4.* introduces PropertyKey and IdentifierName, we now handle these variants of strings.
Attached file Bug 1497446 - mach rust vendor;r?ted (obsolete) —
The previous two changesets bump up a few dependencies. This is the companion mach rust vendor.
Moving from Phabricator.
Attachment #9016260 - Attachment is obsolete: true
Attachment #9016304 - Flags: review?(ted)
Comment on attachment 9016304 [details] [diff] [review]
Bug 1497446 - mach rust vendor.diff

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

This patch appears to be reversed, which makes it a bit confusing.
Attachment #9016304 - Attachment is obsolete: true
Attachment #9016304 - Flags: review?(ted)
Attachment #9016732 - Flags: review?(ted)
Comment on attachment 9016732 [details] [diff] [review]
Bug 1497446 - mach rust vendor.diff

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

From reading over this patch it does the following:

Removes these crates:
- atty 0.1.2

Updates these crates:
binjs_meta 0.3.10 => 0.4.3
bit-set 0.4.0 => 0.5.0
bit-vec 0.4.4 => 0.5.0
ena 0.5.0 => 0.9.3
lalrpop 0.15.2 => 0.16.0
lalrpop-snap 0.15.2 => 0.16.0
lalrpop-util 0.15.2 => 0.16.0
webidl 0.6.0 => 0.8.0
Attachment #9016732 - Flags: review?(ted) → review+
Wow, I hadn't used checkin-needed in ages. Let's see if that still works :)
Keywords: checkin-needed
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f5207ccd12f
Porting binsource generator to binjs_meta 0.4.3. r=efaust
https://hg.mozilla.org/integration/mozilla-inbound/rev/006cd7f96ea3
Implementing read[Maybe]{IdentifierName, PropertyKey} in BinTokenReader*. r=efaust
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c4f6cc4fccf
mach rust vendor. r=ted
Keywords: checkin-needed
Backed out for build bustages.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=busted&selectedJob=205148231&revision=0c4f6cc4fccf38369baf6dcce5aea235ad55aa96

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=205148231&repo=mozilla-inbound&lineNumber=236

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/c55cc49537553ce267068a89e7046c3f90cc3c81

[task 2018-10-12T19:58:47.744Z] 19:58:47     INFO -      self._gen_rust_rules(obj, backend_file)
[task 2018-10-12T19:58:47.744Z] 19:58:47     INFO -    File "/builds/worker/workspace/build/src/python/mozbuild/mozbuild/backend/tup.py", line 1025, in _gen_rust_rules
[task 2018-10-12T19:58:47.744Z] 19:58:47     INFO -      '\n'.join(output_lines))
[task 2018-10-12T19:58:47.744Z] 19:58:47     INFO -  Exception: cargo --build-plan failed with output:
[task 2018-10-12T19:58:47.744Z] 19:58:47     INFO -  error: the lock file needs to be updated but --frozen was passed to prevent this
[task 2018-10-12T19:58:48.226Z] 19:58:48     INFO -  *** Fix above errors and then restart with\
[task 2018-10-12T19:58:48.226Z] 19:58:48     INFO -                 "./mach build"
[task 2018-10-12T19:58:48.226Z] 19:58:48     INFO -  client.mk:124: recipe for target 'configure' failed
[task 2018-10-12T19:58:48.227Z] 19:58:48     INFO -  make: *** [configure] Error 1
[task 2018-10-12T19:58:48.329Z] 19:58:48    ERROR - Return code: 2
Flags: needinfo?(dteller)
Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/966e9ad9b487
Implementing read[Maybe]{IdentifierName, PropertyKey} in BinTokenReader* r=Yoric
https://hg.mozilla.org/integration/mozilla-inbound/rev/0018705f1d1e
mach rust vendor r=Yoric
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0a8b3b1f341
Porting binsource generator to binjs_meta 0.4.3 r=Yoric
Keywords: checkin-needed
Pushed by aiakab@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15b1e744df85
Porting binsource generator to binjs_meta 0.4.3 r=Yoric
https://hg.mozilla.org/integration/mozilla-inbound/rev/133dc138e871
Implementing read[Maybe]{IdentifierName, PropertyKey} in BinTokenReader* r=Yoric
https://hg.mozilla.org/integration/mozilla-inbound/rev/9343eb2d9c82
mach rust vendor r=Yoric
(In reply to Noemi Erli[:noemi_erli] from comment #23)
> Backed out changeset c0a8b3b1f341 (Bug 1497446) for build bustages CLOSED
> TREE
> 
> Push with failures:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&resultStatus=testfailed,busted,
> exception&classifiedState=unclassified&selectedJob=205284097&revision=c0a8b3b
> 1f341f355c389b1a331456c8ab42ca472
> 
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=205284097&repo=mozilla-
> inbound&lineNumber=4298
> 
> Backout:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 161107a152cc4e09d1deba2d0303d6e97c1aa8b0
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 0ee1b8cbc8c835c2ad3debecac916ee2d28c5bba

This looks like it was built with two of my patches but not the third.

What's going on?
Flags: needinfo?(dteller) → needinfo?(nerli)
Was an import issue with mercurial and it didn't want to back out an empty patch. Has been relanded with the third patch correctly imported. Sorry for the confusion.
Flags: needinfo?(nerli)
In the future you may want to leave a comment on the checkin-needed explaining what the reviewer line should be. These pushes with r=Yoric caught my eye as suspicious. :-)
(In reply to David Teller [:Yoric] (please use "needinfo") from comment #1)
> Oh, great:
> «
> The following files exceed the filesize limit of 102400:
> 
> third_party/rust/lalrpop/src/parser/lrgrammar.rs
> »

What happened here? This file should have been vendored as it is part of lalrpop-snap 0.16.0 but it's not in the tree. Now anybody that runs |./mach vendor rust| on the tree runs into this problem. Nathan, can I land this file with the --build-peers-said-large-imports-were-ok flag?
Flags: needinfo?(nfroyd)
Flags: needinfo?(dteller)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #29)
> 
> What happened here? This file should have been vendored as it is part of
> lalrpop-snap 0.16.0 but it's not in the tree. Now anybody that runs |./mach
> vendor rust| on the tree runs into this problem. Nathan, can I land this
> file with the --build-peers-said-large-imports-were-ok flag?

As far as I understand, this file is *not* part of lalrpop-snap, but is an artifact of bug 1461553.

At least, removing my ~/.cargo/registry seemed to not include this file.
Flags: needinfo?(dteller)
I confirmed that the crate for lalrpop-snap 0.16.0 does actually include that file:

curl --location https://crates.io/api/v1/crates/lalrpop-snap/0.16.0/download > lalrpop-snap.tgz
tar xzf lalrpop-snap.tgz
wc -c lalrpop-snap-0.16.0/src/parser/lrgrammar.rs

--> shows lalrpop-snap-0.16.0/src/parser/lrgrammar.rs with a size of 3887355 bytes
It's quite possible that whoever packaged and published the crate had a copy of lrgrammar.rs in their source directory, and so it got packaged up along with everything else. `cargo publish` tends to do that (in the past it's slurped up random dll files and other junk).
Left a comment at https://github.com/lalrpop/lalrpop/issues/409#issuecomment-430195104 - you might want to reopen that issue (I don't have permission to)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #29)
> (In reply to David Teller [:Yoric] (please use "needinfo") from comment #1)
> > Oh, great:
> > «
> > The following files exceed the filesize limit of 102400:
> > 
> > third_party/rust/lalrpop/src/parser/lrgrammar.rs
> > »
> 
> What happened here? This file should have been vendored as it is part of
> lalrpop-snap 0.16.0 but it's not in the tree. Now anybody that runs |./mach
> vendor rust| on the tree runs into this problem. Nathan, can I land this
> file with the --build-peers-said-large-imports-were-ok flag?

So...the github issue says that lrgrammar.rs isn't necessary in 0.15.2, it's still not necessary in 0.16.0 and has been removed, but our vendoring of 0.16.0 didn't include the file, and this is causing problems somehow?  Things seem to build OK with it?  (Or maybe they don't, because we're not actually testing binast in automation?)  I am very confused.

r=me if lrgrammar.rs is actually needed for binast to build, but I'd really like to understand what the problem is here.
Flags: needinfo?(nfroyd)
I think that:
(a) the file is not necessary in order to build (i haven't confirmed this but it seems plausible)
(b) the file was supposed to be removed in the 0.16.0 crate but wasn't
(c) our vendoring of 0.16.0 didn't include the file even though it was in the crate
(d) our build is fine because even though the file is missing, it's not actually needed
(e) our vendoring is not fine because the file is in the crate but not vendored. so running |./mach vendor rust| on a clean tree produces an error when it tries to add the file

If they can produce a 0.16.1 package with the file omitted then we could just update to that, which is probably a better solution as it avoids adding the file to our tree at all. But they should do it quickly because I'm sure somebody will be running |./mach vendor rust| any day now...
More discovery shows that the lrgrammar.rs file used to be vendored in m-c, until it was *removed* in this bug. Presumably that was an accident.

https://hg.mozilla.org/mozilla-central/diff/9343eb2d9c82/third_party/rust/lalrpop-snap/src/parser/lrgrammar.rs

So adding it back should be the right thing to do. Discussion on https://github.com/lalrpop/lalrpop/issues/409#issuecomment-430208382 seems to indicate that the file is a necessary component of lalrpop-snap, although since m-c builds fine without it, I question whether we even are even building/using lalrpop-snap at all, or if it is just getting pulled in because of overly-broad dependency chains.
This (large) file exists in the lalrpop-snap crate and should be part of
the vendoring of that crate. However it seems to have been accidentally
removed in bug 1497446. This patch adds it back by running
  ./mach vendor rust --build-peers-said-large-imports-were-ok
on a clean m-c tree.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3361fcc40ea2
Follow-up to repair incorrect vendoring of lalrpop-snap. r=froydnj
Attachment #9016260 - Attachment is obsolete: false
Attachment #9016260 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.