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: relaxed condition to check if resource exists #1621

Merged
merged 1 commit into from
Nov 10, 2022
Merged

fix: relaxed condition to check if resource exists #1621

merged 1 commit into from
Nov 10, 2022

Conversation

kittaakos
Copy link
Contributor

Motivation

To open built-in examples from an AppImage.

Change description

Relaxed the condition to check if a resource exists.
The original (fs-extra-based) implementation did not check if the file is writable either.
Resources are not writable in mounted AppImages.

Other information

Closes #1586

Reviewer checklist

  • PR addresses a single concern.
  • The PR has no duplicates (please search among the Pull Requests before creating one)
  • PR title and description are properly filled.
  • Docs have been added / updated (for bug fixes / features)

The original (`fs-extra`-based) implementation did not check if the
file is writable either.

Resources are not writable in mounted AppImages.

Closes #1586

Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
@kittaakos kittaakos added topic: code Related to content of the project itself os: linux Specific to Linux operating system type: imperfection Perceived defect in any part of project labels Nov 3, 2022
@kittaakos kittaakos marked this pull request as ready for review November 7, 2022 10:41
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

I verified this fixes #1586 for the Linux AppImage package.

Thanks Akos!

I was never able to reproduce #1586 with the Linux ZIP package, so I can't verify it fixes the reported problem as it applies to that package. I do suspect that the cause was the same (e.g., perhaps there is a way to run the IDE from inside the ZIP package without extracting the archive?), and therefore will also be fixed by this PR.

@kittaakos
Copy link
Contributor Author

I was never able to reproduce #1586 with the Linux ZIP package,

Me neither, but I thought it was an app image issue. Has anybody had the same issue with the ZIP?

so I can't verify it fixes the reported problem as it applies to that package. I do suspect that the cause was the same (e.g., perhaps there is a way to run the IDE from inside the ZIP package without extracting the archive?), and therefore will also be fixed by this PR.

I learned this the hard way. If the app image is started, the AppDir is mounted read-only. If you start the ZIP image, the contained resources (such as the built-ins) are resolved with the absolute FS path instead of the mounted one. From here:

root INFO cloneExample file:///tmp/.mount_arduin6hMpJz/resources/app/node_modules/arduino-ide-extension/Examples/01.Basics/AnalogReadSerial

VS

root INFO cloneExample file:///home/parallels/Desktop/dev/arduino-ide/electron/build/dist/linux-unpacked/resources/app/node_modules/arduino-ide-extension/Examples/01.Basics/AnalogReadSerial

Since it was read-only, the exists logic gave a false negative result, so the built-in sketch creations failed with the sketch does not exist error when IDE2 was started as an app image.

@per1234
Copy link
Contributor

per1234 commented Nov 7, 2022

Has anybody had the same issue with the ZIP?

Yes, the original reporter:

#1586 (comment)

@per1234 you changed my report to add the line:

The bug does not occur when using the ZIP package of the IDE.

but this is incorrect. ALL of my testing was done with the zip package. I never tested with the appimage

@kittaakos kittaakos mentioned this pull request Nov 7, 2022
3 tasks
@kittaakos kittaakos added the status: on hold Do not proceed at this time label Nov 8, 2022
@kittaakos
Copy link
Contributor Author

kittaakos commented Nov 10, 2022

OP of #1586 has confirmed that it worked with the ZIP. So only the AppImage was affected. I am removing the on hold label.

@kittaakos kittaakos removed the status: on hold Do not proceed at this time label Nov 10, 2022
Copy link
Contributor

@AlbyIanna AlbyIanna left a comment

Choose a reason for hiding this comment

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

Neat! Code is 👍

@kittaakos kittaakos merged commit cc2d557 into main Nov 10, 2022
@kittaakos kittaakos deleted the #1586 branch November 10, 2022 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
os: linux Specific to Linux operating system topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Built-in" examples fail to open
3 participants