-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
build: Allow setting flags from the environment #957
Conversation
LDFLAGS
from environment
🤔 So far, any build variable was reset on purpose so that any setting that a user or a distro may have will not cause any issue with Unikraft builds. The idea behind this prevention mechanism is that these flags may be preset for application builds intended for the Linux host system. There is no guarantee that these flags are compatible with Unikraft. However, it is a question if this is a common scenario and if it is worth to protect against it. |
I wrote my opinion already on discord but I'll repeat it here: We should not worry too much about about dealing with broken setups. If a distro sets these variables everywhere, then the distro made some dubious design choices. This would also interfere with other build systems and would risk breaking them (For example, setting the It is possible that a user sets these variables, but then we should probably also assume that they could have a modified/broken toolchain installed. If you think that this is a serious concern that there are many users settings these variables on system-level then we should probably also request the output of detailed diagnostic script on issues instead. |
My experience aligns with @mschlumpp's judgement. I have maintained quite a few Arch Linux user packages in the past. On Arch, there are system-wide When I looked into injecting On the other hand, I completely understand @skuenzer's worry about existing global flags unconsciously affecting Unikraft builds, too. I don't have as much experience with other distros as I have with Arch, but I would expect them to behave similarly. That means, they only set In the end, though, I would be happy with both |
6d3179c
to
050622c
Compare
LDFLAGS
from environment
I changed this PR to introduce What do you think, @mschlumpp and @skuenzer? :) |
I added |
Thanks for your reply, @skuenzer! :) I renamed it to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkroening I would add all the variables in this pr under the help prompt (maybe a new section similar to command-line variables
)
At least the UK_LDEPS
one is not that intuitive, so we need it documented.
e812ccd
to
cafa9bd
Compare
Thanks, @StefanJum, I added them to help. :) |
Why do some of the vars have an underscore and others not? |
I am happy to add underscores, but they were missing from @mschlumpp's initial suggestion. :) |
I would say with underscore it's easier to read. |
Thanks for the input! I added the underscore. :) |
This adds `UK_ASFLAGS`, `UK_CFLAGS`, `UK_CXXFLAGS`, `UK_GOCFLAGS`, and `UK_LDFLAGS` for explicitly setting these build flags for Unikraft. For reference, see - https://github.com/torvalds/linux/blob/v6.4/Documentation/kbuild/kbuild.rst#environment-variables - https://github.com/torvalds/linux/blob/v6.4/Makefile#L1097-L1101 Signed-off-by: Martin Kröning <[email protected]>
This adds support for the `UK_LDEPS` environment variable. This variable specifies external files that are watched for changes by make. If any of the specified files change, the final linking step is redone. The files are only watched and not consumed to avoid changing the order of supplied `LDFLAGS`. This was inspired by `cargo:rerun-if-changed=PATH`: - https://doc.rust-lang.org/cargo/reference/build-scripts.html#rerun-if-changed Signed-off-by: Martin Kröning <[email protected]>
Beep boop! I ran Unikraft's
Truncated logs starting from first warning 817064d:
View complete logs | Learn more about Unikraft's coding style and contribution guidelines. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good, thanks.
Reviewed-by: Stefan Jumarea [email protected]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Reviewed-by: Marco Schlumpp [email protected]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved-by: Simon Kuenzer [email protected]
This adds support for the `UK_LDEPS` environment variable. This variable specifies external files that are watched for changes by make. If any of the specified files change, the final linking step is redone. The files are only watched and not consumed to avoid changing the order of supplied `LDFLAGS`. This was inspired by `cargo:rerun-if-changed=PATH`: - https://doc.rust-lang.org/cargo/reference/build-scripts.html#rerun-if-changed Signed-off-by: Martin Kröning <[email protected]> Reviewed-by: Stefan Jumarea <[email protected]> Reviewed-by: Marco Schlumpp <[email protected]> Approved-by: Simon Kuenzer <[email protected]> Tested-by: Unikraft CI <[email protected]> GitHub-Closes: #957
Prerequisite checklist
checkpatch.uk
on your commit series before opening this PR;Base target
Additional configuration
Description of changes
This adds
UK_ASFLAGS
,UK_CFLAGS
,UK_CXXFLAGS
,UK_GOCFLAGS
, andUK_LDFLAGS
for explicitly setting these build flags for Unikraft.For reference, see
This also adds
UK_LDEPS
for manually relinking if user-specified files change. This has been separated to avoid tampering with the order ofLDFLAGS
.Inspired by