Skip to content

Comments

fix: copy bundled JLink to tmp before installing#1255

Open
boundlesscalm wants to merge 1 commit intomainfrom
fix/ubuntu-bundled-jlink
Open

fix: copy bundled JLink to tmp before installing#1255
boundlesscalm wants to merge 1 commit intomainfrom
fix/ubuntu-bundled-jlink

Conversation

@boundlesscalm
Copy link
Contributor

This is necessary when installing the bundled JLink as it otherwise resides in a user created space which the root user is blocked from accessing.

This is necessary when installing the bundled JLink as it otherwise
resides in a user created space which the root user is blocked from
accessing.
@boundlesscalm boundlesscalm added doc not required All PRs either need "doc required" or "doc not required". ui not required All PRs either need "ui required" or "ui not required". labels Feb 12, 2026
@boundlesscalm
Copy link
Contributor Author

We could technically just do this for all OS' but I don't see a reason for that except saving us like 3 lines of code.

Copy link
Contributor

@KievDevel KievDevel left a comment

Choose a reason for hiding this comment

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

Missing cleanup logic: will the installer be removed after installation from tmp? Or if app will be closed unexpectedly in between on the next launch of the launcher..?

@datenreisender
Copy link
Contributor

@KievDevel @boundlesscalm: You could simplify the logic a bit and also get automatic cleanup with createDisposableTempDir(), as e.g. it is done at

using tempDir = createDisposableTempDir();
await moveFile(appPath, path.join(tempDir.path, 'to-be-deleted'));
};
.

With resource management you have to be a bit careful at what point the resource goes out of scope and thus can be disposed but I think this should be fine to use here. If you want to, I can also sketch an implementation or we can pair on it.

@datenreisender
Copy link
Contributor

We could technically just do this for all OS' but I don't see a reason for that except saving us like 3 lines of code.

While slightly leaning towards the simpler code, both are fine for me: Doing this only for Linux or for all OS’.

@boundlesscalm
Copy link
Contributor Author

We can't use the shared createDisposableTempDir since that creates a user owned directory and we run into the same issue.
But removing the file we just copied after installing it should not be an issue.

Not sure if we have to be so explicit in removing it anyway. It's in the /tmp/ folder which is expected to be automatically cleaned in ubuntu and most other OS-es.

@datenreisender
Copy link
Contributor

We can't use the shared createDisposableTempDir since that creates a user owned directory and we run into the same issue.

Ah, I thought it is currently only a problem because it is mounted through FUSE, not because the ownership and permissions on the directory. We could also change ownership and permissions on the directory, seems like the easier approach to me, but another solution that works is also ok.

But removing the file we just copied after installing it should not be an issue.

Not sure if we have to be so explicit in removing it anyway. It's in the /tmp/ folder which is expected to be automatically cleaned in ubuntu and most other OS-es.

We do not strictly have to clean it up but I still think it is good practice to do it, also given that the temp folders are usually only wiped on startup.

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

Labels

doc not required All PRs either need "doc required" or "doc not required". ui not required All PRs either need "ui required" or "ui not required".

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants