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 --no-sandbox to Linux Launch #262

Merged
merged 1 commit into from
Dec 25, 2022
Merged

Add --no-sandbox to Linux Launch #262

merged 1 commit into from
Dec 25, 2022

Conversation

confused-Techie
Copy link
Member

While I know there were talks about properly detecting what Linux Graphic Drivers were in use, and much more elegant solutions to this problem.

But realistically as those haven't been done yet, I feel it's likely in our best interest to find a solution to this problem rather than the perfect one.

That is until someone can come along and do this much more properly, which I highly implore anyone else to.

But otherwise this change will likely resolve the following:

Copy link
Member

@Spiker985 Spiker985 left a comment

Choose a reason for hiding this comment

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

Looks great and is the proper location for the calls

Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

I also am pretty sure this is what should change to fix the flags for pulsar.sh on Linux, so... Approved!

@DeeDeeG
Copy link
Member

DeeDeeG commented Dec 25, 2022

By the way, the main process of Atom/Pulsar is pretty sophisticated about handing off launches of new main processes to be handled by the existing one instead... I wonder why we need this launcher at all??? I just about always use the absolute path to the real pulsar binary, since the .sh launcher confuses me and I reflexively don't trust extra complexity that I don't understand...

🤷

(But strictly regarding whether or not to merge this PR, please ignore. Just taking a step back to look at the larger picture.)

@confused-Techie
Copy link
Member Author

By the way, the main process of Atom/Pulsar is pretty sophisticated about handing off launches of new main processes to be handled by the existing one instead... I wonder why we need this launcher at all??? I just about always use the absolute path to the real pulsar binary, since the .sh launcher confuses me and I reflexively don't trust extra complexity that I don't understand...

Yeah some aspects here do seem a little weird, especially considering this launcher seems to be only used on Linux and MacOS, so obviously Pulsar can handle it's own when launching on Windows, so weird that this isn't done here.

We could always investigate simplifying this to just rely on the main process itself, but wonder if we would lose being able to do stuff like this PR does.


But otherwise thanks for the approvals guys! I'll go ahead and merge this one

@confused-Techie confused-Techie merged commit 996c411 into master Dec 25, 2022
@mauricioszabo
Copy link
Contributor

@confused_techie and @DeeDeeG, windows by definition never locks the terminal when you're running a GUI app, where Linux and Mac do. This shell script basically avoids locking the terminal.

More complex answer - the main entry point of a Mac and Linux binaries are always the same, regardless if it's a graphical or console app - on these systems, all apps start the same but they connect with the graphics server after launch. On Windows, there are two different entry points of the app, one for consoles and other for graphical ones. That's why it never locks the terminal - because, by all means, that app ISN'T a terminal app, so Thera why Windows doesn't need the sh file

@confused-Techie
Copy link
Member Author

@mauricioszabo Super interesting insight, but so that does mean we have to keep the script around. But thanks for taking the time to explain

@Spiker985 Spiker985 deleted the no-sandbox branch December 25, 2022 04:48
@Daeraxa
Copy link
Member

Daeraxa commented Dec 27, 2022

Just to check, this PR is only valid for .deb and .rpm releases isn't it?

Not sure how to do this with the .appimage, the internal .desktop already has --no-sandbox but this doesn't seem to work when running it, do we need to include this in the apprun or something? The tarball is another story as well that I'm not sure what we do about.

@mauricioszabo
Copy link
Contributor

@Daeraxa we can always add --no-sandbox into the yarn start script, or maybe "patch" the cirrus to add these on Linux targets...

@Daeraxa
Copy link
Member

Daeraxa commented Dec 27, 2022

Yeah i was going to look at adding it to the yarn script when i got back as well as experimenting with the appimage.

@DeeDeeG
Copy link
Member

DeeDeeG commented Dec 28, 2022

While I know there were talks about properly detecting what Linux Graphic Drivers were in use, and much more elegant solutions to this problem.

For the record, and not to bump a closed PR thread, but I am of the belief this is not at all related to which graphics card or which driver you have. It affects all Linux distros with the clone3 system call enabled + Electrons older copy of Chromium that bug s out with that system call.

atom/atom#23036 (comment)

Happens for me on an old intel machine with an with integrated GPU and happens for @Spiker985 on NVidia discrete graphics, apparently. Haven't had a chance to test on AMD graphics, but I don't think that's the thing that makes this issue happen. I think it's a CPU code path coincidentally interfering with GPU process, but again I think that's a coincidence.

EDIT: Eh, some people in the OpenSuse bug report say their nvidia driver had the issue while their intel driver didn't. Maybe the clone3 system call is used by GPU drivers? I dunno. Anyhow, it affects quite a few people. And I somehow got it to happen in a fairly ancient intel-only machine. So who knows? 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants