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

add default-dir build option #141

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dweiller
Copy link
Contributor

@dweiller dweiller commented Jul 19, 2024

Extracted from #139.

This PR add a new build option -Ddefault-dir that sets the default installation directory. It defaults to "zig" which means that this PR does not change any behaviour if the option is not provided to zig build.

@marler8997
Copy link
Owner

Hmmm, I think people have requested more flexibility here, namely, the ability to set the default install directory to be relative to different locations. Maybe something like this?

-Ddefault-install-dir=(ZIGUP_EXE_DIR)/zig
-Ddefault-install-dir=(HOME)/zigup

@dweiller
Copy link
Contributor Author

Hmmm, I think people have requested more flexibility here, namely, the ability to set the default install directory to be relative to different locations. Maybe something like this?

-Ddefault-install-dir=(ZIGUP_EXE_DIR)/zig
-Ddefault-install-dir=(HOME)/zigup

Is there an issue/PR where this has been discussed, so I know the scope of what people have been asking for? For something more complex than just changing the subdirectory that the current master branch uses I don't think a single build option with an embedded DSL is the right way to go. I see a few systems that could be reasonable supported and chosen between by build options:

  1. A static path relative to some parent. This is the current behaviour of master and the current PR where the parent depends on the OS ($HOME for Linux/macOS and the directory zigup is in for Windows). If the only additional flexibility desired for the -Ddefault-dir option is to make this choice of runtime-resolved parent configurable I'd suggest an additional -Dinstall-parent option that takes an enum { exe_dir, home } argument, and perhaps also allowing -Ddefault-dir to be an absolute path.
  2. XDG base spec (see Add support for installing compilers to XDG_DATA_HOME #142)
  3. Environment variable (with fall-back to one of the above), but I think you're not in support of this option (I found Environment variables for configuration #91 (comment))

@dweiller
Copy link
Contributor Author

  1. A static path relative to some parent. This is the current behaviour of master and the current PR where the parent depends on the OS ($HOME for Linux/macOS and the directory zigup is in for Windows). If the only additional flexibility desired for the -Ddefault-dir option is to make this choice of runtime-resolved parent configurable I'd suggest an additional -Dinstall-parent option that takes an enum { exe_dir, home } argument, and perhaps also allowing -Ddefault-dir to be an absolute path.

I've implemented this, but I haven't added these options to the CI testing (and since I don't have a windows machine to test on there's always a chance I messed up the implementation of -Dinstall-parent=home on windows). I'll have to take a look at how the CI testing works before this should be merged.

@dweiller dweiller marked this pull request as draft July 21, 2024 05:40
@marler8997
Copy link
Owner

This is a strategy I can get behind.

@dweiller
Copy link
Contributor Author

@marler8997 What are your thoughts on testing these build options in CI? The current CI setup uses the --install-dir flag to force installation to a scratch directory, except on Windows where the binary is already in the scratch directory (and the install dir is in the binaries parent). However, the idea of using a separate scratch directory doesn't work for testing -Dinstall-parent=home without some sort of chroot jail or changing $HOME/%HOMEPATH% for the test process so as to not mess with a local developers home directory if they run zig build ci locally. The same issue comes up with adding tests for #142.

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