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

--release=x ignored when preferred_optimize_mode is set #19732

Open
17dec opened this issue Apr 22, 2024 · 4 comments · May be fixed by #19733
Open

--release=x ignored when preferred_optimize_mode is set #19732

17dec opened this issue Apr 22, 2024 · 4 comments · May be fixed by #19733

Comments

@17dec
Copy link

17dec commented Apr 22, 2024

Zig Version

0.13.0-dev.4+c7ffdbcd4

Steps to Reproduce and Observed Behavior

I had set preferred_optimize_mode in ncdu's build.zig in order to provide a good default when building in release mode, but this causes a custom --release=x flag to be ignored:

$ zig build --release=fast
$ sha1sum zig-out/bin/ncdu
644d71472a8515c9676adcb0a743988e31ad5f25  zig-out/bin/ncdu
$ zig build --release=safe
$ sha1sum zig-out/bin/ncdu
644d71472a8515c9676adcb0a743988e31ad5f25  zig-out/bin/ncdu

(Throwing away all build files and clearing caches in between the two builds doesn't affect the result)

Expected Behavior

--release=x option is respected.

@17dec 17dec added the bug Observed behavior contradicts documented or intended behavior label Apr 22, 2024
@Vexu Vexu added this to the 0.13.0 milestone Apr 22, 2024
@ifreund ifreund linked a pull request Apr 22, 2024 that will close this issue
@andrewrk andrewrk removed the bug Observed behavior contradicts documented or intended behavior label Apr 22, 2024
@andrewrk
Copy link
Member

I wrote this on the PR but I think here is better:

This is already working as designed, although clearly the flag names do not make this self-evident. How it currently stands is that there are two parties communicating their preference to the build system:

  • upstream maintainer communicates preference via preferred_optimize_mode when calling standardOptimizeOption.
  • the person running zig build communicates default preference via --release flag.

The build system gives the upstream maintainer's preference priority of these two options. However, the application may also expose additional options. --release is intended to be globally set by the distribution in the package management code, for example maybe they hard-code --release=safe as the global default. Meanwhile, any particular project may expose more fine-grained options, including perhaps the familiar -Doptimize flag, which take the most precedence of all and will override --release.

@nektro
Copy link
Contributor

nektro commented Apr 22, 2024

--release is intended to be globally set by the distribution in the package management code

if this is true I think the issue should stand and the upstream's preferences should never take priority over this flag. imo it should go

  1. -Doptimize/-Dmode
  2. --release=X
  3. preferred_optimize_mode

preferred_optimize_mode should only be taken if a bare --release with no preference given is put

@tekktonic
Copy link

tekktonic commented Apr 22, 2024

(Moving my comment from IRC.)
I suspect the sensible way to do this is to rename "preferred" to "override" or "force" or something similar and emit a warning onto stderr when --release is ignored. Possibly with an additional actual "preferred" to be used when --release isn't given (or we get a bare release without an argument)?

@judofyr
Copy link

judofyr commented Apr 25, 2024

Another alternative: Remove --release=x and only support --release. That makes the flag a very basic request from the caller: "I'm building this package for release". The package maintainer is the best person to set sensible defaults for this scenario. If the caller also wants to make more fine-tuned tweaks, then they need to get into -Doptimize and/or other flags and really understand the build system for this particular project.

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

Successfully merging a pull request may close this issue.

6 participants