Skip to content

Conversation

@rockwotj
Copy link
Contributor

@rockwotj rockwotj commented Feb 5, 2025

If you just want to add a single flag (that is per platform) and leave the rest of the default
flags, you have to copy out all the existing flags and add it to what
you have. Instead support just adding an extra flag that is appended to
the default flags.

@rockwotj
Copy link
Contributor Author

rockwotj commented Feb 5, 2025

Example of where I'd like this extra_flags ability: redpanda-data/redpanda#25042

Copy link
Member

@fmeum fmeum left a comment

Choose a reason for hiding this comment

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

@jsharpe @rrbutani Even though this adds many attributes, they are mechanical enough that I'm okay with that. What do you think?

doc = ("Extra compile_flags, added after default values. " +
"`{toolchain_path_prefix}` in the flags will be substituted by the path " +
"to the root LLVM distribution directory. Provide one list for each " +
"target OS and arch pair you want to override " +
Copy link
Member

Choose a reason for hiding this comment

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

override is misleading here, we are just adding flags.

If you just want to add a single flag (that is per platform) and leave the rest of the default
flags, you have to copy out all the existing flags and add it to what
you have. Instead support just adding an extra flag that is appended to
the default flags.
@fmeum fmeum enabled auto-merge (squash) February 19, 2025 07:13
@rockwotj
Copy link
Contributor Author

The alternative to all the extra flags is a way to "inject" the initial flags when providing the override flags, maybe via some magic value like $ORIGINAL_VALUES?

@rockwotj rockwotj closed this Mar 12, 2025
auto-merge was automatically disabled March 12, 2025 19:02

Pull request was closed

@fmeum
Copy link
Member

fmeum commented Mar 13, 2025

@rockwotj Sorry, I didn't notice that auto-merge didn't go through (likely because of the unresolved comment). Feel free to reopen and I can merge - I think that more attributes are better than magic values in old ones.

fmeum pushed a commit that referenced this pull request Sep 30, 2025
If you just want to add a single flag (that is per platform) and leave
the rest of the default
flags, you have to copy out all the existing flags and add it to what
you have. Instead support just adding an extra flag that is appended to
the default flags.

This is a resubmit of #452 by @rockwotj with minor fixes in
documentation.

---------

Co-authored-by: Tyler Rockwood <[email protected]>
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.

2 participants