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

ecs-settings-applier: conditional compilation #1008

Conversation

samuelkarp
Copy link
Contributor

Description of changes:
This commit will compile ecs-settings-applier with a no-op body when any variant other than aws-ecs-1 is used. This allows for the top-level cargo make targets like unit-tests to function properly for all variants.

Testing done:

  • cargo check with a variety of different VARIANT values provided, ensuring that cargo only rebuilds when the variant changes and builds the appropriate code
  • cargo make unit-tests with -e BUILDSYS_VARIANT set to different variants, ensuring that compilation does not fail.

Note: rustc warns for unused imports, constants, structs, and functions when a variant other than aws-ecs-1 is used. If we want to reduce the set of warnings, I'd appreciate suggestions on the best way to conditionally define those items (other than sprinkling #[cfg(variant="aws-ecs-1")] all over the place).

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

@samuelkarp samuelkarp requested a review from tjkirch August 5, 2020 18:50
@samuelkarp samuelkarp requested review from webern and bcressey August 5, 2020 18:51
@samuelkarp samuelkarp force-pushed the ecs-settings-applier-conditional-compile branch from 74b557b to 23e9bf7 Compare August 5, 2020 19:19
Copy link
Contributor

@tjkirch tjkirch left a comment

Choose a reason for hiding this comment

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

Nice find with rustc-cfg! Is it possible to do the conditionalizing at a higher level than the run function, so the whole crate isn't compiled? We know we're not including this binary, so it feels silly to build it, even if it wouldn't do anything when run. (It'd get rid of the warnings too...)

Copy link
Contributor

@webern webern left a comment

Choose a reason for hiding this comment

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

Makes sense.

@webern
Copy link
Contributor

webern commented Aug 5, 2020

Nice find with rustc-cfg! Is it possible to do the conditionalizing at a higher level than the run function, so the whole crate isn't compiled? We know we're not including this binary, so it feels silly to build it, even if it wouldn't do anything when run. (It'd get rid of the warnings too...)

I was trying to find something like this, too. I was thinking at the workspace level, but I did not find anything that could conditionally leave the crate out of the workspace.

@tjkirch
Copy link
Contributor

tjkirch commented Aug 5, 2020

I was trying to find something like this, too. I was thinking at the workspace level, but I did not find anything that could conditionally leave the crate out of the workspace.

I couldn't find anything with a quick look, but what if we put all of the code in a module and #[cfg] that, rather than just having #[cfg] on run? Then none of the other structs/functions/etc. would compile, either.

A couple thoughts around options:

  • src/ecs.rs, with all the code from main.rs, and have main.rs just do something like #[cfg(ecs)] use ecs::*?
  • symlink strategy, like models, so it'd be based on variant, and we could actually just have an empty file for any other variant. Downside: still need directories/etc. for any new variant, even though they wouldn't care about this crate.

@samuelkarp
Copy link
Contributor Author

src/ecs.rs, with all the code from main.rs, and have main.rs just do something like #[cfg(ecs)] use ecs::*?

I think I like this one better than symlinks. We'll still end up building the crate as empty, but it'll get rid of the warnings at least.

@samuelkarp samuelkarp force-pushed the ecs-settings-applier-conditional-compile branch from 23e9bf7 to f386163 Compare August 5, 2020 22:43
This commit will compile ecs-settings-applier with a no-op body when any
variant other than aws-ecs-1 is used.  This allows for the top-level
`cargo make` targets like `unit-tests` to function properly for all
variants.
@samuelkarp samuelkarp force-pushed the ecs-settings-applier-conditional-compile branch from f386163 to 0366b80 Compare August 5, 2020 22:55
@samuelkarp
Copy link
Contributor Author

@tjkirch GitHub doesn't show the diff well, but src/main.rs was moved to src/ecs.rs, slightly modified to remove the #[cfg] lines and make main() public, and a new src/main.rs was added that calls the old one.

@samuelkarp samuelkarp merged commit 75fc15f into bottlerocket-os:develop Aug 6, 2020
@samuelkarp samuelkarp deleted the ecs-settings-applier-conditional-compile branch August 6, 2020 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants