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)
Core
JavaScript Engine
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.
Assignee | ||
Comment 1•6 years ago
|
||
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.
Assignee | ||
Comment 2•6 years ago
|
||
So, we're currently blocked by https://github.com/sgodwincs/webidl-rs/pull/19, which I opened a few hours ago.
Updated•6 years ago
|
Priority: -- → P3
Assignee | ||
Updated•6 years ago
|
Summary: [BinAST] Move to binjs_meta 0.4.1 → [BinAST] Move to binjs_meta 0.4.2
Assignee | ||
Comment 3•6 years ago
|
||
https://github.com/sgodwincs/webidl-rs/pull/19 unblocked
Assignee | ||
Comment 4•6 years ago
|
||
Now blocked on https://github.com/lalrpop/lalrpop/issues/409 .
Assignee | ||
Comment 5•6 years ago
|
||
Ok, unblocked.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → dteller
Summary: [BinAST] Move to binjs_meta 0.4.2 → [BinAST] Move to binjs_meta 0.4.3
Assignee | ||
Comment 7•6 years ago
|
||
Since binjs_meta 0.4.* introduces PropertyKey and IdentifierName, we now handle these variants of strings.
Assignee | ||
Comment 8•6 years ago
|
||
Depends on D8298
Assignee | ||
Comment 9•6 years ago
|
||
The previous two changesets bump up a few dependencies. This is the companion mach rust vendor.
Assignee | ||
Comment 10•6 years ago
|
||
Moving from Phabricator.
Attachment #9016260 -
Attachment is obsolete: true
Attachment #9016304 -
Flags: review?(ted)
Assignee | ||
Comment 11•6 years ago
|
||
Moved over from Phabricator (already reviewed): https://bugzilla.mozilla.org/attachment.cgi?id=9016078&action=edit
Attachment #9016078 -
Attachment is obsolete: true
Attachment #9016306 -
Flags: review+
Assignee | ||
Comment 12•6 years ago
|
||
Moved over from phabricator. Already reviewed: https://bugzilla.mozilla.org/attachment.cgi?id=9016079&action=edit
Attachment #9016079 -
Attachment is obsolete: true
Attachment #9016307 -
Flags: review+
Comment 13•6 years ago
|
||
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.
Assignee | ||
Comment 14•6 years ago
|
||
Attachment #9016304 -
Attachment is obsolete: true
Attachment #9016304 -
Flags: review?(ted)
Attachment #9016732 -
Flags: review?(ted)
Comment 15•6 years ago
|
||
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+
Assignee | ||
Comment 16•6 years ago
|
||
Wow, I hadn't used checkin-needed in ages. Let's see if that still works :)
Keywords: checkin-needed
Updated•6 years ago
|
Comment 17•6 years ago
|
||
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
Comment 18•6 years ago
|
||
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)
Assignee | ||
Comment 19•6 years ago
|
||
Attachment #9016306 -
Attachment is obsolete: true
Flags: needinfo?(dteller)
Attachment #9016957 -
Flags: review+
Assignee | ||
Comment 20•6 years ago
|
||
Attachment #9016307 -
Attachment is obsolete: true
Attachment #9016958 -
Flags: review+
Assignee | ||
Comment 21•6 years ago
|
||
Attachment #9016732 -
Attachment is obsolete: true
Attachment #9016959 -
Flags: review+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 22•6 years ago
|
||
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
Comment 23•6 years ago
|
||
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=c0a8b3b1f341f355c389b1a331456c8ab42ca472 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
Flags: needinfo?(dteller)
Comment 24•6 years ago
|
||
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
Comment 25•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0018705f1d1e https://hg.mozilla.org/mozilla-central/rev/15b1e744df85 https://hg.mozilla.org/mozilla-central/rev/133dc138e871 https://hg.mozilla.org/mozilla-central/rev/9343eb2d9c82
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Assignee | ||
Comment 26•6 years ago
|
||
(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)
Comment 27•6 years ago
|
||
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)
Comment 28•6 years ago
|
||
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. :-)
Comment 29•6 years ago
|
||
(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)
Assignee | ||
Comment 30•6 years ago
|
||
(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)
Comment 31•6 years ago
|
||
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
Comment 32•6 years ago
|
||
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).
Comment 33•6 years ago
|
||
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)
Comment 34•6 years ago
|
||
(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)
Comment 35•6 years ago
|
||
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...
Comment 36•6 years ago
|
||
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.
Comment 37•6 years ago
|
||
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.
Comment 38•6 years ago
|
||
Pushed by kgupta@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3361fcc40ea2 Follow-up to repair incorrect vendoring of lalrpop-snap. r=froydnj
Comment 39•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3361fcc40ea2
Updated•6 years ago
|
Attachment #9016260 -
Attachment is obsolete: false
Updated•5 years ago
|
Attachment #9016260 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•