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

fix(windows): Fix Windows System User build failures by using the current directory for bundling tools (fix: 9895) #9914

Merged
merged 7 commits into from
Jun 3, 2024

Conversation

AnthonyNGarcia
Copy link

@AnthonyNGarcia AnthonyNGarcia commented May 30, 2024

Closes #9895 with details on the issue there.

As a summary: Tauri uses Wix/NSIS toolsets for bundling .msi and .exe files respectively. Currently, it installs/retrieves these toolchains from the user app directory. However, this approach fails if the current user is a System user, since their app directory (the system directory) is restricted, which results in the bundling failing due to unusable Wix/NSIS executables.

This PR solves the problem by instead installing/retrieving the Wix/NSIS toolset from the current project directory, in target/tools. For example, Wix would be at target/tools/Wix. This would:

  • Be a known location that should be accessible, since it is the current project's workspace.
  • Enable deterministic builds across different Windows Users, since the tools are in the project's workspace rather than being user-specific
  • Enable controlled builds for the same Windows User across different Tauri projects, in case support comes later to use a specific version of these tools for a specific project.
  • Better support as-is offline builds wherein these tools can be configured in the project's workspace ahead-of-time (rather than configuring them on-the-fly).

Tested this code change locally:

  • cargo test
  • cargo clippy
  • Using it on my Windows machine with Tauri Quickstart guide example, to generate .msi and .exe (tests Wix and NSIS flows, respectively). They both built successfully. Also confirmed both of them work for app installation

@AnthonyNGarcia AnthonyNGarcia requested a review from a team as a code owner May 30, 2024 02:04
Copy link
Member

@amrbashir amrbashir left a comment

Choose a reason for hiding this comment

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

We used to use target directory before but we changed it to use cache_dir because these files rarely change so it makes sense to just cache them for every project and the few files that change, we detect that and update them.

That said, I'd like to keep the current behavior, and add this either as a fallback when current cache_dir fails, or as a separate CLI flag (or environment flag)

@FabianLars
Copy link
Member

Agreed with Amr, just wanted to mention that this would need a proper target dir detection (similar to how the bundle/ folder gets placed) because <cwd>/target/ is simply incorrect tbh.

…ting the fix behind a Tauri configuration feature flag, default false
@AnthonyNGarcia
Copy link
Author

Great callout, thank you folks. I like the suggestion to keep the existing behavior as default given it is more optimal for most use cases. I've put the feature behind a feature flag in the Tauri config json with schema validation. I believe this approach accomplishes the best of both worlds: gating the feature, but also minimizing risk of build errors associated with CLI usage and flags, by leveraging the feature via schema validation. After all, someone who needs this feature needs this all the time, so setting it once in the config and being safe would be preferred (at least speaking from the voice of one such person who wants to consume this fix).

Tested that the new json config property is fully optional. If not provided at all, defaults to false (aka existing behavior). Also tested if false is explicitly provided (again, existing behavior). Finally, when true is provided (new behavior, working).

As for the callout on getting to target directory more correctly, good callout. I updated it to traverse up one from the available project_out_directory. Do let me know if you are aware of a better way to get to the target directory. I also considered just putting these tools directly in project_out_directory but it name conflicts with existing folders there wix and nsis which are holding intermediate artifacts.

@AnthonyNGarcia
Copy link
Author

AnthonyNGarcia commented May 31, 2024

As per offline discussion in Discord, moved the feature from bundle.windows up to bundle directly as it could apply to other platforms (though this PR just implements it for Windows). Updated docs accordingly and renamed tool folder from tools -> .tauri-tools

@FabianLars
Copy link
Member

I also considered just putting these tools directly in project_out_directory but it name conflicts with existing folders there wix and nsis which are holding intermediate artifacts.

Hmm, maybe something like this (copy pasted from somewhere else in the cli)

  let mut path = get_cargo_metadata()
    .with_context(|| "failed to get cargo metadata")?
    .target_directory;

Using the parent dir of project_out_directory is probably a bit awkward because it will be different if the user uses the --target flag, duplicating the tools at which point you may as well use project_out_directory directly (with the extra .tauri-tools folder) just to be safe.

@amrbashir amrbashir merged commit a301be5 into tauri-apps:1.x Jun 3, 2024
32 checks passed
@AnthonyNGarcia AnthonyNGarcia deleted the 1.x branch June 3, 2024 22:48
@MystiPanda
Copy link

MystiPanda commented Jun 4, 2024

I just tested this PR and didn't make any changes to tauri.conf.json, I got permissions error on GitHub Actions:
https://github.com/clash-verge-rev/clash-verge-rev/actions/runs/9360177885/job/25765128485
image

@MystiPanda
Copy link

Well, unrelated, I found out that #9902 caused the problem.

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.

4 participants