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 initial support for link flag #403

Merged
merged 11 commits into from
Apr 27, 2024
Merged

Add initial support for link flag #403

merged 11 commits into from
Apr 27, 2024

Conversation

tiziodcaio
Copy link
Contributor

@tiziodcaio tiziodcaio commented Oct 2, 2023

The link flag implement a mechanism (only for linux at the moment) to avoid downloading firefox from the mozilla server. Instead it will symlink to the distro installed one

  • Embrace more cases (recognise developer-edition and others archs).
  • I need help for fixing the connector in the GUI, at the moment is broken.
  • TUI works well.

Copy link
Owner

@filips123 filips123 left a comment

Choose a reason for hiding this comment

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

Symlinks also work on other systems (although I think they require admin on Windows), so the Linux check should probably be removed. Then, the default source path for the runtime should also be updated to detect paths on other OSes.

To allow different Firefox installations (ESR, Developer Edition, Nightly, etc.) in different directories, it might be useful to change the --link argument to allow an optional value to specify the source path. When the path is specified, it should be linked instead of the default path. When it is not, the default path (see above) should be used. This probably also means that the source path also needs to be stored somewhere in the config and configurable through the extension settings.

Another thing is that the extension setup wizard should be updated to allow users to link the runtime instead of downloading it. The extension could also hardcode a few known paths for different Firefox versions on different OSes so users wouldn't have to manually search the path, but also still allow manual input in case the path is different. However, this can be done later.


I don't have much time currently, but I hope I will be able to review PR more next week.


This isn't directly related to your PR, but another way of "linking" the global runtime is using FUSE OverlayFS (#67, #214). So, in the future, another option similar to --link (maybe --overlay) could be added to use FUSE OverlayFS to link the runtime.

native/src/components/runtime.rs Outdated Show resolved Hide resolved
Comment on lines 774 to 872
// Listen for use Linked runtime
document.getElementById('settings-use-linked-runtime').addEventListener('change', async function () {
config.use_linked_runtime = this.checked
await setConfig(config)
})
Copy link
Owner

Choose a reason for hiding this comment

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

For the extension settings, you can check how settings like settings-always-patch are implemented. So, this code should be inside setTimeout(async () => { (around line 875).

@tiziodcaio
Copy link
Contributor Author

Symlinks also work on other systems (although I think they require admin on Windows), so the Linux check should probably be removed. Then, the default source path for the runtime should also be updated to detect paths on other OSes.

Well.. At the moment I can work into a Linux OS only, because of this I inserted the checks... I thought that was the simplest method to implement without breaking it in macOS (you need more work because of the .app bundle) and Windows (symbolic and hard links needs the administrator privileges)

To allow different Firefox installations (ESR, Developer Edition, Nightly, etc.) in different directories, it might be useful to change the --link argument to allow an optional value to specify the source path. When the path is specified, it should be linked instead of the default path. When it is not, the default path (see above) should be used. This probably also means that the source path also needs to be stored somewhere in the config and configurable through the extension settings.

Yeah... I really want to make the thing more flexible, because at the moment I tested from Arch Linux only, and that's because it needs a bit of testing and it's immature merge yet in my opinion.

Another thing is that the extension setup wizard should be updated to allow users to link the runtime instead of downloading it. The extension could also hardcode a few known paths for different Firefox versions on different OSes so users wouldn't have to manually search the path, but also still allow manual input in case the path is different. However, this can be done later.

I thought about that, but I went into the backend firstly, so I've not studied how to implement into the extension yet

This isn't directly related to your PR, but another way of "linking" the global runtime is using FUSE OverlayFS (#67, #214). So, in the future, another option similar to --link (maybe --overlay) could be added to use FUSE OverlayFS to link the runtime.

Oh. I didn't see it. Interesting idea... But I'm not really know well the OverlayFS technology so I will wait someone more expert 😉

@filips123
Copy link
Owner

Well.. At the moment I can work into a Linux OS only, because of this I inserted the checks... I thought that was the simplest method to implement without breaking it in macOS (you need more work because of the .app bundle) and Windows (symbolic and hard links needs the administrator privileges)

I almost forgot macOS uses .app bundles and requires special handling... So, it's probably fine if support for macOS is added later.

And for Windows, other types of shortcuts/links are supported, and some of them appearently don't require admin privileges. For example, Rustup uses junctions instead of symlinks on Windows to prevent requiering admin. However, it uses a lot of low-level unsafe code and direct calls to Windows API, which doesn't seem like a good solution, so this can also wait until there is a better one.

Also, another thing related to symlinks/junctions is that we need to check what happens when they are deleted. Will it only remove the symlink itself or also try to delete the actual content? This should be checked for normal deleting through the file manager, shell and functions that are used in the native program (remove_dir_all and remove_dir_contents).

I thought about that, but I went into the backend firstly, so I've not studied how to implement into the extension yet

Yeah, the extension stuff (mainly the setup wizard) can also wait until the native implementation is ready.

@VarLad
Copy link

VarLad commented Oct 23, 2023

@filips123 (apologies for the out of context question)
But I wonder if this PR would help in alleviating the challenges in packaging PWAsForFirefox as a flatpak extension?

@filips123
Copy link
Owner

@VarLad The only thing it would help with is that you wouldn't need to have two instances of Firefox installed (which is not really required but can still be useful, even without Flatpak). However, Flatpak still can't be supported properly until they add support for Native Messaging API.

@VarLad
Copy link

VarLad commented Oct 24, 2023

@filips123 Why not package this as a flatpak extension?

@filips123
Copy link
Owner

I'm still not sure how useful would that be and it it could completely work. The native program needs some additional permissions (to create desktop files and icons) and I don't know if this can work as an extension. Additionally, it makes it hard to use alternative browser (either as a main browser or as an app runtime).

But I can check if it can be done. If it can work at least partially, it's probably still better than nothing, at least until native messaging is properly supported.

@tiziodcaio
Copy link
Contributor Author

tiziodcaio commented Oct 29, 2023

Another problem would be the update of firefox, since we should check if the binary is going to be updated. Maybe an hard link instead of softlink on the firefox & firefox-bin..? I will try in these days

@filips123 filips123 mentioned this pull request Dec 11, 2023
@tiziodcaio
Copy link
Contributor Author

tiziodcaio commented Jan 22, 2024

I think is more mature now, I cleaned up a bit. What do you think it is missing to be merged with experimental support?

  • The binary won't update with distro packages and meanwhile it will break itself. Maybe implement some kind of checksum and replace? Or hard link?
  • Add to the extension GUI some options.
  • Add support to:
    • Windows
    • macOS
    • BSD (test)

@filips123
Copy link
Owner

Why is copying binaries required? Does it not work when using symlinks?

The binary won't update with distro packages and meanwhile it will break itself.

This is probably the main thing that should be implemented before this can be merged.

Normal symlinks would probably be the best option if you can make them work. Hard links are also probably quite easy to implement, but don't work across partitions (I think), which might be a problem on systems that have a separate /home partition. I don't know if there are any other "easy" options. Maybe the program could check if the source binaries have different modification time and copy them right before launching the web app (as it's already done for profile and runtime patching).

It also shouldn't be that hard to implement BSD support. I think there are a still few places left which limit this to Linux and should be fixed. Also, I think that Firefox on BSD is in /usr/local/lib/firefox/ (this needs to be checked), so the source path needs to change dependeng on the OS. Support for other OSes can be added in the future.

Also regarding paths, it would be nice if the source path can be configured by the user (see my first comment). However, in this case, the path might also need to be stored in the config, which is probably more work, so it can be done later.

Extension UI can probably also be added later, or I can implement it myself (as it's a bit harder now because of localization).

@tiziodcaio
Copy link
Contributor Author

Binaries has to be copied due to firefox, which at the moment it starts itself it grabs its configs from the folder in which it was put, symlinks are not enough. Copies are only 700KB, so is not a problem of weight, but only in cases of updating it might break, so as we said we need to think how to implement it.

I discarded hard links because they are harder to understand, and I am the first with a separated home partition (already tested and yes, because of that it cannot work).

We should check more paths in which Firefox might be installed, like flatpak, snaps, distros and BSDs.

Source path should not be neither complicated to implement, it got lost in the comments.

I am agreeing on extension. Maybe keeping it to test in tui mode it would the best idea at the moment.

So we will keep at the moment this feature unix-only, then if we have time we can think about windows and finally MacOS (is the most complicated with all the bundles thing in my opinion)

@filips123
Copy link
Owner

Binaries has to be copied due to firefox, which at the moment it starts itself it grabs its configs from the folder in which it was put, symlinks are not enough. Copies are only 700KB, so is not a problem of weight, but only in cases of updating it might break, so as we said we need to think how to implement it.

Then, unless there is some trick to make Firefox work with symlinks, this could probably be solved if the program checks if binaries are outdated every time when launching a web app and copy them again when needed.

For example, a similar thing is already used to check if the patches should be applied.

In this case, the program could do something like this:

  1. Do this inside SiteLaunchCommand.
  2. Check if use_linked_runtime is enabled.
  3. If enabled, get the source Firefox path (which probably needs to be stored in the config if it is configurable by the user).
  4. Then, check file modification dates for binaries in the source and target directories.
  5. If the source dates are newer than existing target ones, copy them over.

Also, eventually, I would like to get this work even when immutable-runtime is enabled. One purpose of immutable-runtime feature is that when distros provide Firefox with their own custom patches which are necessary to run it (like on Nix and Guix, see #204), users can't accidentally overwrite it with the normal Mozilla version with firefoxpwa runtime install. Then, distros would probably provide their runtime in a global /usr/share/firefoxpwa/runtime. In this case, the link option is still useful when users want to link another Firefox version (still with distro patches though), and should then link the runtime into user-specific directory which takes precedence over the global one.

However, to make this work, I will also need to change how the runtime directory is determined, so it's out of scope for this PR. For now, it's fine if linking is disabled when immutable-runtime is enabled.

@tiziodcaio

This comment was marked as outdated.

@tiziodcaio tiziodcaio marked this pull request as ready for review February 8, 2024 16:28
@tiziodcaio
Copy link
Contributor Author

There are still some problems with linking... Re putting in draft

@tiziodcaio tiziodcaio marked this pull request as draft February 8, 2024 17:45
@filips123
Copy link
Owner

I probably won't have much time in the next two weeks, but then I can review and probably merge it.

BROKEN
- I need help for fixing the connector in the GUI. The TUI works well.
Added "support" to BSD, needed testing
Disabled when using immutable-runtime feature
Removed/cleaned some branches of rust code for readability
Because otherwise it will fail due to existance of old files
@tiziodcaio tiziodcaio marked this pull request as ready for review March 22, 2024 14:40
@filips123
Copy link
Owner

filips123 commented Mar 28, 2024

There are some clippy and compile errors on Windows and macOS, mostly because of runtime.link which doesn't exist on those systems.

There is also a compile error on Linux, but it is unrelated to this PR, and I don't know yet why it happens...

Also, I reverted the extension change for now.

@tiziodcaio
Copy link
Contributor Author

Great! I decided to push the changes onto a feature, and I fixed the clippy errors on macOS/windows!

@filips123
Copy link
Owner

I will try to fix a few remaining CI failures, and then I think this initial support should be ready. In the future, I will also change how detecting runtime works (as I mentioned before), but it might take some time and I don't know when it will be ready.

Also, should this feature be enabled by default in the release builds, or should it be disabled until support is improved (with support for custom paths, other systems and extension UI)?

Copy link
Owner

@filips123 filips123 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 this initial version is fine. I will probably keep it disabled by default for now though, so you will have to compile it manually to enable the feature.

@filips123 filips123 changed the title Add link flag Add initial support for link flag Apr 27, 2024
@filips123 filips123 merged commit ea94d33 into filips123:main Apr 27, 2024
18 checks passed
@filips123 filips123 added this to the 2.12.0 milestone May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants