refactor: change project flag configuration behavior #1451
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Fixes `(Project.get-config "cflags")` so that it actually returns the project's C flag field. Previously it was returning the project's preprocessor calls.
This commit alters the project configuration compiler flag setting behavior to allow users to override default project flags. Flag setting commands (cflag, libflag, pkgconfigflag) now take lists of strings as arguments. Previously, they were append-only and accepted string arguments. Now, to override flags, users can call: ``` (Project.get-config "cflag") => (...a bunch of default platform flags; "-wall" ...) (Project.config "cflag" '("-my" "-new" "-flags")) (Project.get-config "cflag") => ("-my" "-new" "-flags") ``` In order to append flags, users now need to `cons` to a `config-get`: ``` (Project.config "cflag" (cons "-new" (Project.get-config "cflag"))) ``` BREAKING CHANGE: Code that relies on the previous behavior will break, as calling these functions with non-list arguments is now an error. I considered a softer path whereby we'd overload the current behavior to avoid breaking existing users; however, the result would be confusing. Passing a list would result in overriding the project field while passing a single string would result in an append. A user might conceivably wonder why passing a list doesn't result in a multi-append. Ultimately I felt making a breaking change would be less confusing than this compatibility preserving behavior down the line. Provides convenience macros for appending flags to project flag fields cflag, libflag, and pkgconfigflag.
eriksvedang approved these changes Jan 30, 2023
Collaborator
eriksvedang left a comment
eriksvedang left a comment There was a problem hiding this comment.
Looks good! Seems to break the tests though?
Updates some project configuration calls in the Core library to follow the new configuration behavior (pass complete flag sets and not individual strings).
To account for the new flag setting configuration behavior.
Adds a preload eval to set the -Wnounknown-warning-option flag in clang -- this way, when different versions don't support our default flag set, they can still run tests. Otherwise passing an unknown flag to clang results in a compilation error.
Contributor Author
| Ok, so this PR gets our failing *nix builds green again by bypassing |
Contributor Author
| I'm wondering if we should just drop |
Member
| Possibly a good idea, but maybe we should also just remove usages of |
Contributor Author
Even better! |
Collaborator
| Does this need to pull in master for the tests to pass? |
Collaborator
| @scolsen Should we try to get this merged? |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
This PR changes the behavior of the project flag setting dynamic functions. Instead of appending a single string to the flagset, they now take a list of strings, which serve as the new flagset:
In order to append flags, users now need to
consto aconfig-get:This is a breaking change, as passing a single string is now an error.
I considered overloading the behavior as an alternative so that we wouldn't break existing callers, but I feel this would ultimately lead to unintuitive behavior (calling with a single string appends while calling with a list of strings overrides ... users might wonder why the list doesn't append all the strings in the list instead).
This PR also fixes a bug w/ fetching project C flags and provides helper macros for appending flags to existing project flag sets given the new behavior.