Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom features no longer affect the build #21314

Closed
jdm opened this issue Aug 1, 2018 · 12 comments
Closed

Custom features no longer affect the build #21314

jdm opened this issue Aug 1, 2018 · 12 comments
Assignees
Labels

Comments

@jdm
Copy link
Member

@jdm jdm commented Aug 1, 2018

When I write ./mach build -d --features whatever, I no longer receive an error about the whatever feature not existing. If I add -v to the build, I don't see whatever passed to any of the crates. This regressed sometime in the past month.

@jdm jdm added A-mach A-build labels Aug 1, 2018
@highfive
Copy link

@highfive highfive commented Aug 1, 2018

@jdm
Copy link
Member Author

@jdm jdm commented Aug 1, 2018

The actual build command being invoked is rustup run --install nightly-2018-07-17 cargo build -v --features whatever, so I'm really confused why this is being ignored now.

@jdm
Copy link
Member Author

@jdm jdm commented Aug 2, 2018

I believe this was regressed by #20912, which removed the use of --manifest-path from the cargo commands. When I create a workspace with a single default-member, cargo build --features whatever no longer produces an error. When I run cargo build --manifest-path project --features whatever (assuming the default member is named project), I receive the expected error.

cc @SimonSapin

@jdm
Copy link
Member Author

@jdm jdm commented Aug 2, 2018

This also means that we can't use --debug-mozjs, or any of the VR-related features.

@jdm
Copy link
Member Author

@jdm jdm commented Aug 2, 2018

I've filed rust-lang/cargo#5849.

@jdm
Copy link
Member Author

@jdm jdm commented Aug 13, 2018

@SimonSapin Are you aware of a workaround here? Could we restore the previous manifest-path usage that existed before #20912?

@jdm jdm closed this Aug 13, 2018
@jdm jdm reopened this Aug 13, 2018
@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Aug 14, 2018

I’m not sure exactly. I think that reverting to using --manifest-path in mach would fix command-line usage of --feature, but then we’d need to find another way to deal with libsimpleservo v.s. the servo executable. Perhaps the former should not be in default-members after all, and instead we should have (more) custom logic in mach for Android targets.

@jdm
Copy link
Member Author

@jdm jdm commented Sep 5, 2018

Simon said they would work on this.

@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Sep 10, 2018

Using --manifest-path means picking one "root" crate to build, whereas the current default is to build both ports/servo and ports/libsimpleservo.

@paulrouget What do you think of building the latter for Android targets and the former in other cases? Are there cases where we want to make the other choice? Specifically, since ports/servo is an empty crate on Android, do we want to support (building) libsimpleservo on non-Android? Would ./mach build --manifest-path ports/libsimpleservo be an acceptable interface for that?

@jdm
Copy link
Member Author

@jdm jdm commented Sep 10, 2018

We could do something like the old ./mach build-geckolib, too. We're relying on being able to build libsimpleservo for ML devices, but those aren't detected as android devices.

@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Sep 10, 2018

What are ML devices in this context?

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Sep 11, 2018

@SimonSapin libsimpleservo is not android specific (there's a non-jni version too), even though we only use it for Android now. We need to be able to build both ports.

SimonSapin added a commit that referenced this issue Sep 25, 2018
… --libsimpleservo`

Fixes #21314
bors-servo added a commit that referenced this issue Sep 25, 2018
Only build ports/servo by default (except Android), add `./mach build --libsimpleservo`

Fixes #21314
SimonSapin added a commit that referenced this issue Sep 25, 2018
bors-servo added a commit that referenced this issue Sep 25, 2018
Only build ports/servo by default (except Android), add `./mach build --libsimpleservo`

Fixes #21314

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21809)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Sep 25, 2018
Only build ports/servo by default (except Android), add `./mach build --libsimpleservo`

Fixes #21314

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21809)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Sep 26, 2018
Only build ports/servo by default (except Android), add `./mach build --libsimpleservo`

Fixes #21314

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21809)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Sep 26, 2018
Only build ports/servo by default (except Android), add `./mach build --libsimpleservo`

Fixes #21314

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21809)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Sep 27, 2018
Only build ports/servo by default (except Android), add `./mach build --libsimpleservo`

Fixes #21314

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21809)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Sep 27, 2018
Only build ports/servo by default (except Android), add `./mach build --libsimpleservo`

Fixes #21314

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21809)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.