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

Enable building features in crates on iOS #5944

Conversation

buggmagnet
Copy link
Contributor

@buggmagnet buggmagnet commented Mar 12, 2024

This PR will enable us building rust crate features by passing a second argument to the build-rust-library.sh script.

For example
build-rust-library.sh mullvad-api api-override

If we need to build more than one feature flag, we can pass in a single argument, all the features separated by spaces
build-rust-library.sh mullvad-api "featureA featureB featureC"


This change is Reviewable

@buggmagnet buggmagnet added the iOS Issues related to iOS label Mar 12, 2024
@buggmagnet buggmagnet self-assigned this Mar 12, 2024
Copy link

linear bot commented Mar 12, 2024

Copy link
Contributor

@niklasberglund niklasberglund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @buggmagnet)


ios/build-rust-library.sh line 5 at r1 (raw file):

set -euvx

if [ "$#" -gt 2 ]

Nit: the previous condition also guarded against 0 arguments. This condition could for example be [ "$#" -gt 2 ] || [ "$#" -eq 0 ] to also guard against 0 arguments


ios/build-rust-library.sh line 8 at r1 (raw file):

If we need to build more than one feature flag, we can pass in a single argument, all the features separated by spaces
build-rust-library.sh mullvad-api "featureA featureB featureC"

Found this usage information very helpful, should it be added here as well? Without this info and by looking at the help text here I would assume the syntax build-rust-library.sh mullvad-api featureA featureB featureC is valid but it isn't.

Copy link
Contributor Author

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @niklasberglund)


ios/build-rust-library.sh line 8 at r1 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

If we need to build more than one feature flag, we can pass in a single argument, all the features separated by spaces
build-rust-library.sh mullvad-api "featureA featureB featureC"

Found this usage information very helpful, should it be added here as well? Without this info and by looking at the help text here I would assume the syntax build-rust-library.sh mullvad-api featureA featureB featureC is valid but it isn't.

Done.

Copy link
Contributor

@niklasberglund niklasberglund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion

@buggmagnet buggmagnet force-pushed the improve-the-rust-library-build-script-to-pass-feature-flags-ios-551 branch from 03e912e to c29a0ac Compare March 13, 2024 08:45
@rablador rablador requested a review from niklasberglund March 13, 2024 12:14
Copy link
Contributor

@rablador rablador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @niklasberglund)


ios/build-rust-library.sh line 16 at r3 (raw file):

# Enable cargo features by passing feature names to this script, i.e. build-rust-library.sh mullvad-api api-override
# If more than one feature flag needs to be enabled, pass in a single argument all the features flags separated by spaces

nit: "argument of all"?

Copy link
Contributor Author

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @niklasberglund and @rablador)


ios/build-rust-library.sh line 16 at r3 (raw file):

Previously, rablador (Jon Petersson) wrote…

nit: "argument of all"?

I don't think that suggestion is proper english.

"Pass all the feature flags in a single argument"
"Pass in a single argument all the feature flags"

@buggmagnet buggmagnet force-pushed the improve-the-rust-library-build-script-to-pass-feature-flags-ios-551 branch from c29a0ac to 4d7e5ef Compare March 14, 2024 07:36
@buggmagnet buggmagnet merged commit 5208401 into main Mar 14, 2024
5 of 6 checks passed
@buggmagnet buggmagnet deleted the improve-the-rust-library-build-script-to-pass-feature-flags-ios-551 branch March 14, 2024 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants