Skip to content

fix: symlink issue bundling for linux #5781#6391

Merged
lucasfernog merged 2 commits intotauri-apps:devfrom
passivedragon:dev
Mar 17, 2023
Merged

fix: symlink issue bundling for linux #5781#6391
lucasfernog merged 2 commits intotauri-apps:devfrom
passivedragon:dev

Conversation

@passivedragon
Copy link
Contributor

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

Other information

Frankly, this fix is more from @ca0v who suggested it in the issue thread #5781, rather than me just putting it in.

I have test run this on a single project with success, but I haven't tested beyond that.

About the issue:
The bundling script fails if it has run once before, as the links it creates already exist, and it does not like that. The added -f overwrites the existing links with a new one, which should be the desired behavior.

@passivedragon passivedragon requested a review from a team as a code owner March 3, 2023 04:06
@FabianLars
Copy link
Member

a bit weird that this seems to be required since the AppDir is supposed to be cleaned out before that 🤔

@passivedragon
Copy link
Contributor Author

Hmm, that definitely doesn't happen.

line 19 is the first time the directory is mentioned, and all it does is create it if it doesn't exist already
mkdir -p "{{app_name}}.AppDir"

it might need rm -r "{{app_name}}.AppDir/*" in the line directly after that and before the copy instruction, but from what I've seen from testing, that doesn't seem to be necessary.

@FabianLars
Copy link
Member

FabianLars commented Mar 3, 2023

I was talking about the rust code that runs before the bash script is invoked: https://github.com/tauri-apps/tauri/blob/dev/tooling/bundler/src/bundle/linux/appimage.rs#L38

@passivedragon
Copy link
Contributor Author

That's pretty weird, if that failed, it should error and fail then and there to my understanding.
Wouldn't it need to error on bad input? From a quick, shallow read over rust docs seems to me like it might be happier with output_path.clone() instead of &output_path. I'm a rust noob though.

With even just this change for me though, it manages to build all files for me, all the way to the .sig file.

@FabianLars
Copy link
Member

clone vs & won't make a difference but you're right that it should fail if it can't remove the folder, at least the way i understood the docs.

To be clear, i'm fine with merging this PR but i'm irritated that it's needed in the first place (something we probably should look into as i'd imagine that the not deleted folder could cause issues in other places too)

@FabianLars
Copy link
Member

Oh btw, we'd need a change file similar to https://github.com/tauri-apps/tauri/blob/dev/.changes/bundler-msi-mini-version.md

@passivedragon
Copy link
Contributor Author

That work?

@FabianLars
Copy link
Member

yep, works for me thanks

Copy link
Member

@lucasfernog lucasfernog left a comment

Choose a reason for hiding this comment

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

I think the fix doesn't make much sense and the problem might be somewhere else, but I'll take it if it works.

@lucasfernog lucasfernog merged commit 2f70d8d into tauri-apps:dev Mar 17, 2023
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.

3 participants