1370571 - stylo: remove --enable-stylo-build-bindgen

Status: RESOLVED DUPLICATE of bug 1447611

(Firefox Build System :: General, enhancement, P5)

Description

[Nathan Froyd [:froydnj]]

Emilio says we shouldn't need this anymore, and removing it will simplify the number of configurations we have to care about.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: remove --enable-stylo-build-bindgen configure option

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.

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

(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

[Xidorn Quan [:xidorn] UTC+11]

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

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 5

[Nathan Froyd [:froydnj]]

(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.

Comment 6

[Nathan Froyd [:froydnj]]

(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?

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 8

[Bobby Holley (:bholley)]

(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.

Comment 9

[Nathan Froyd [:froydnj]]

(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

[Bobby Holley (:bholley)]

(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.

Comment 11

[Chris Peterson [:cpeterson]]

Removing --enable-stylo-build-bindgen does not need to block building Stylo support in Nightly.

Comment 12

[Chris Peterson [:cpeterson]]

status-firefox57=wontfix unless someone thinks this bug should block 57

Comment 13

[Jan Beich]

Bug 1447611 removed --enable-stylo-build-bindgen.