Closed
Bug 1370571
Opened 7 years ago
Closed 6 years ago
stylo: remove --enable-stylo-build-bindgen
Categories
(Firefox Build System :: General, enhancement, P5)
Firefox Build System
General
Tracking
(firefox57 wontfix)
RESOLVED
DUPLICATE
of bug 1447611
Tracking | Status | |
---|---|---|
firefox57 | --- | wontfix |
People
(Reporter: froydnj, Assigned: froydnj)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
patch
|
xidorn
:
review+
|
Details | Diff | Splinter Review |
Emilio says we shouldn't need this anymore, and removing it will simplify the number of configurations we have to care about.
![]() |
Assignee | |
Comment 1•7 years ago
|
||
This option isn't necessarily for Gecko CI and unnecessarily complicates things; let's just remove it. My understanding is that we don't need this for Servo itself, either, since the necessary files are checked in to Servo nowadays? Please confirm.
Attachment #8874899 -
Flags: review?(xidorn+moz)
![]() |
Assignee | |
Updated•7 years ago
|
Assignee: nobody → nfroyd
Comment 2•7 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #1) > Created attachment 8874899 [details] [diff] [review] > remove --enable-stylo-build-bindgen configure option > > This option isn't necessarily for Gecko CI and unnecessarily complicates > things; let's just remove it. I'm fine with removing this. In the past, people found this to be a useful workaround when they hit problems involving bindgen but they themselves don't do much work with style systems. If we are confident enough that we would no longer have that many issues (via tooling and CI), it is probably fine to remove it. That said, I guess it's probably better leaving it there for a while during our transition to building stylo by default. That would allow people to be unblocked when they hit bindgen issues. If we don't see any complaint against bindgen after that, we may be more confident to remove it. What do you think? > My understanding is that we don't need this for Servo itself, either, since > the necessary files are checked in to Servo nowadays? Please confirm. We still need it for Servo CI. The bindgen feature of servo/components/style crate is for switching whether the build script uses bindgen or use checked-in binding files. If we remove that feature from Servo side, Servo CI would not be able to build it. So I guess that shouldn't be removed.
Comment 3•7 years ago
|
||
Since we cannot remove bindgen feature from Servo side anyway, removing this probably doesn't gain much... I don't think we need to care about the configuration. That should just work as far as Servo CI tests it :)
Comment 4•7 years ago
|
||
Comment on attachment 8874899 [details] [diff] [review] remove --enable-stylo-build-bindgen configure option Review of attachment 8874899 [details] [diff] [review]: ----------------------------------------------------------------- This patch itself looks fine to me, thus r+, but I'm reluctant to land this for now.
Attachment #8874899 -
Flags: review?(xidorn+moz) → review+
![]() |
Assignee | |
Comment 5•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #2) > (In reply to Nathan Froyd [:froydnj] from comment #1) > > My understanding is that we don't need this for Servo itself, either, since > > the necessary files are checked in to Servo nowadays? Please confirm. > > We still need it for Servo CI. The bindgen feature of servo/components/style > crate is for switching whether the build script uses bindgen or use > checked-in binding files. If we remove that feature from Servo side, Servo > CI would not be able to build it. So I guess that shouldn't be removed. This patch doesn't touch any code under servo/, so one can still build Servo using SERVO_TARGET_DIR or what have you.
![]() |
Assignee | |
Comment 6•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #3) > Since we cannot remove bindgen feature from Servo side anyway, removing this > probably doesn't gain much... I don't think we need to care about the > configuration. That should just work as far as Servo CI tests it :) I mean, Servo CI isn't building all of Gecko, right? And it's not expected to?
Flags: needinfo?(xidorn+moz)
Comment 7•7 years ago
|
||
Servo CI isn't, and it's not expected to, right. So we always need a flag to switch bindgen off for Servo CI, and switch it on for Gecko.
Flags: needinfo?(xidorn+moz)
Comment 8•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #7) > Servo CI isn't, and it's not expected to, right. So we always need a flag to > switch bindgen off for Servo CI Specifically, so that it can still build the geckolib target with the checked-in binding files.
![]() |
Assignee | |
Comment 9•7 years ago
|
||
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #8) > (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #7) > > Servo CI isn't, and it's not expected to, right. So we always need a flag to > > switch bindgen off for Servo CI > > Specifically, so that it can still build the geckolib target with the > checked-in binding files. Sure. But those binding files are all checked in on the Servo side of things, right? So Servo can build with those, and doesn't need anything from Gecko itself, right?
Comment 10•7 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #9) > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #8) > > (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #7) > > > Servo CI isn't, and it's not expected to, right. So we always need a flag to > > > switch bindgen off for Servo CI > > > > Specifically, so that it can still build the geckolib target with the > > checked-in binding files. > > Sure. But those binding files are all checked in on the Servo side of > things, right? So Servo can build with those, and doesn't need anything > from Gecko itself, right? IIUC, xidorn is just saying that the cargo machinery still needs bindgen to be optional, so that m-c builds and s/s builds can get different behavior wrt bindgen.
Updated•7 years ago
|
Priority: -- → P5
Comment 11•7 years ago
|
||
Removing --enable-stylo-build-bindgen does not need to block building Stylo support in Nightly.
Comment 12•7 years ago
|
||
status-firefox57=wontfix unless someone thinks this bug should block 57
status-firefox57:
--- → wontfix
Updated•7 years ago
|
Product: Core → Firefox Build System
Comment 13•6 years ago
|
||
Bug 1447611 removed --enable-stylo-build-bindgen.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•