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

build: Allow setting flags from the environment #957

Closed
wants to merge 2 commits into from

Conversation

mkroening
Copy link
Member

@mkroening mkroening commented Jun 23, 2023

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran the checkpatch.uk on your commit series before opening this PR;
  • Updated relevant documentation.

Base target

  • Architecture(s): N/A
  • Platform(s): N/A
  • Application(s): N/A

Additional configuration

  • N/A

Description of changes

This adds UK_ASFLAGS, UK_CFLAGS, UK_CXXFLAGS, UK_GOCFLAGS, and
UK_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 of LDFLAGS.

Inspired by

@mkroening mkroening requested a review from a team as a code owner June 23, 2023 10:01
@mkroening mkroening changed the title Makefile: Allow LDFLAGS from environment build: Allow LDFLAGS from environment Jun 23, 2023
@unikraft-bot unikraft-bot added the area/makefile Part of the Unikraft Makefile build system label Jun 23, 2023
@skuenzer
Copy link
Member

🤔 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.
An alternative solution could be to utilize Make's origin function. We could only accept modifications coming from the command line.
Another alternative could be to expect input from a UK_ prefixed version of the variable, like UK_LDFLAGS, ... .
What do you and others think?

@mschlumpp
Copy link
Member

any setting that a user or a distro may have will not cause any issue with Unikraft builds

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 Debug build-type in CMake would be broken if the distro injects optimization flags into environment flags). So far I have only seen distributions that set these kind of flags in their packaging process and do not spray them everywhere.

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.

@mkroening
Copy link
Member Author

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 CFLAGS, CXXFLAGS, etc., but those are only set when building an arch package (see makepkg—Building optimized binaries). If for a certain package some user configured flags would not work, the package maintainer would make sure to work around that by, e.g., clearing the flags in the package build script.

When I looked into injecting LDFLAGS into Unikraft, I originally assumed, this should work already, since this is how I would usually adjust flags when building packages.

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 CFLAGS consciously on a per package basis and not globally. If a user configured global CFLAGS themselves, they might run into all sorts of problems with all kinds of software they compile, which does not mean, Unikraft should necessarily be one of them.

In the end, though, I would be happy with both LDFLAGS or UK_LDFLAGS. Both would work for me. UK_LDFLAGS might cause fewer accidents, but would also be harder to discover.

@mkroening mkroening force-pushed the env-ldflags branch 2 times, most recently from 6d3179c to 050622c Compare June 27, 2023 09:38
@mkroening mkroening changed the title build: Allow LDFLAGS from environment build: Allow setting flags from the environment Jun 27, 2023
@mkroening
Copy link
Member Author

I changed this PR to introduce UKASFLAGS, UKCFLAGS, UKCXXFLAGS, UKGOCFLAGS, and UKLDFLAGS to be set by the environment.

What do you think, @mschlumpp and @skuenzer? :)

@mkroening mkroening requested review from a team as code owners June 28, 2023 08:36
@mkroening
Copy link
Member Author

I added UK_RELINK_IF_CHANGED as per discussion on Discord.

@razvand razvand assigned skuenzer and unassigned nderjung Jul 5, 2023
@razvand razvand requested review from mschlumpp and vladandrew and removed request for a team and adinasm July 5, 2023 09:48
@razvand razvand added this to the v0.14.0 (Prometheus) milestone Jul 5, 2023
@mkroening
Copy link
Member Author

Thanks for your reply, @skuenzer! :)

I renamed it to UK_LDEPS.

Copy link
Member

@StefanJum StefanJum left a 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.

@unikraft-bot unikraft-bot added area/plat Unikraft Patform plat/kvm Unikraft for KVM plat/xen Unikraft for Xen labels Jul 26, 2023
@mkroening mkroening force-pushed the env-ldflags branch 3 times, most recently from e812ccd to cafa9bd Compare July 26, 2023 14:31
@mkroening
Copy link
Member Author

Thanks, @StefanJum, I added them to help. :)

@nderjung
Copy link
Member

Why do some of the vars have an underscore and others not?

@mkroening
Copy link
Member Author

I am happy to add underscores, but they were missing from @mschlumpp's initial suggestion. :)

@mschlumpp
Copy link
Member

I would say with underscore it's easier to read.

@mkroening
Copy link
Member Author

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 <martin.kroening@eonerc.rwth-aachen.de>
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 <martin.kroening@eonerc.rwth-aachen.de>
@unikraft-bot
Copy link
Member

⚠️ Checkpatch passed with warnings

Beep boop! I ran Unikraft's checkpatch.pl support script on your pull request but it encountered warnings:

SHA commit checkpatch
817064d build: Allow setting flags from the environment ⚠️
698866d build: Add `UK_LDEPS` ⚠️

Truncated logs starting from first warning 817064d:

WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#13: 
- https://github.com/torvalds/linux/blob/v6.4/Documentation/kbuild/kbuild.rst#environment-variables

total: 0 errors, 1 warnings, 26 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/tmp/build/a53d4c5b/patches/0001-build-Allow-setting-flags-from-the-environment.patch has style problems, please review.

NOTE: Ignored message types: ASSIGN_IN_IF FILE_PATH_CHANGES NEW_TYPEDEFS OBSOLETE

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

View complete logs | Learn more about Unikraft's coding style and contribution guidelines.

Copy link
Member

@StefanJum StefanJum left a 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 stefanjumarea02@gmail.com

Copy link
Member

@mschlumpp mschlumpp left a 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 marco@unikraft.io

@mkroening mkroening mentioned this pull request Aug 3, 2023
4 tasks
Copy link
Member

@skuenzer skuenzer left a 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 simon@unikraft.io

unikraft-bot pushed a commit that referenced this pull request Aug 4, 2023
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 <martin.kroening@eonerc.rwth-aachen.de>
Reviewed-by: Stefan Jumarea <stefanjumarea02@gmail.com>
Reviewed-by: Marco Schlumpp <marco@unikraft.io>
Approved-by: Simon Kuenzer <simon@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #957
@unikraft-bot unikraft-bot added the ci/merged Merged by CI label Aug 4, 2023
@mkroening mkroening deleted the env-ldflags branch August 4, 2023 10:52
@razvand razvand mentioned this pull request Jan 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/makefile Part of the Unikraft Makefile build system area/plat Unikraft Patform ci/merged Merged by CI kind/enhancement New feature or request plat/kvm Unikraft for KVM plat/xen Unikraft for Xen topic/build Topics to do with the build system
Projects
Status: Done
Status: Done
Status: Done!
Development

Successfully merging this pull request may close these issues.

None yet

7 participants