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

Fix #24 #355, Add compile flag abstractions #457

Closed
wants to merge 1 commit into from

Conversation

skliper
Copy link
Contributor

@skliper skliper commented Jan 7, 2020

Describe the contribution
Fixes #24, Fixes #355

Adds STRICT_NO_WARNINGS and OMIT_DEPRECATED
prep options for CI and as example build

Testing performed
Built with STRICT_NO_WARNINGS=true, false, and undefined
Built with OMIT_DEPRECATED=true, false, and undefined (note sample_app fails)

Confirmed flags get globally applied (not great scoping, but ensures flag coverage)

Expected behavior changes
Simplifies/enables CI based on abstracted flags, see nasa/cFS#39

System(s) tested on:

  • cFS Dev Server 2
  • OS: Ubuntu 18.04
  • Versions: Master bundle with this change

Contributor Info
Jacob Hageman - NASA/GSFC

@skliper skliper added this to the 6.8.0 milestone Jan 7, 2020
Copy link
Contributor

@jphickey jphickey left a comment

Choose a reason for hiding this comment

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

The change seems OK for CI but I am not sure we should necessarily put this type of thing into the example file that we suggest projects base their build off of.

It has been also discussed to set up CI to test multiple different configurations. If we had that feature then this extra flag logic could be confined to those CI-specific test configs, keeping the example version cleaner/simpler.

@skliper
Copy link
Contributor Author

skliper commented Jan 8, 2020

Preference is for complexity to be shifted to the build system vs CI to simplify user's burden to execute similar tests locally (and adapt the sample as needed). Our CI has external dependencies that the build system doesn't have, if a user want's to try out the examples it's trivial and obvious when provided as part of the build system (just flags to prep).

@jphickey
Copy link
Contributor

jphickey commented Jan 8, 2020

Understood, but CI needs to be more extensive anyway, because it currently does not sufficiently cover the compile-time options available. This has been discussed in the past and also I believe there are some related tickets to test with different configs.

My suggestion is to make more than one "defs" directory here, so the sample_defs (which the guides suggest new users clone as their starting point) can remain simple and straightforward.

The other "defs" directories can be used during CI as well as examples of more complex configs.

But the main issue is the logic being proposed is something very CI-specific, and absolutely NOT a good example of the right way to do things.

@skliper
Copy link
Contributor Author

skliper commented Jan 8, 2020

CCB - 20200108: discussed compiler flags in depth, scoping for mission/host vs target would improve things, Joe will submit an update

Adds STRICT_NO_WARNINGS and OMIT_DEPRECATED
prep options for CI and as example build
@skliper
Copy link
Contributor Author

skliper commented Jan 8, 2020

Took a shot at better scoping, and applied by default. Note this just defines HOST_STRICT_COMPILE and TARGET_STRICT_COMPILE, the add_definitions would need to go in the appropriate locations.

@skliper skliper removed this from the 6.8.0 milestone Jan 9, 2020
@skliper
Copy link
Contributor Author

skliper commented Jan 9, 2020

Rejected in favor of #462 and #463

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants