Rework setting PATH variable to not store in a File temporarily #576

jonahgraham · 2023-06-21T18:23:43Z

The old code placed multiple entries for adding to PATH in a File temporarily to get the absolute path. This worked generally on non-Windows platforms because often there was only a single entry.

However on Windows there are generally two entries. Adding to that is the UNC path names that are sometimes present on Windows and the temporary use of File causes corruption in the settings.

Instead collect the new PATH entries in a list of strings, and then before making the final PATH setting, run each individual entry through new File(entry).getAbsolutePath()

TODO:

needs testing - this first version is a quick attempt to understand problem
I wrote this on Linux, needs testing on some other platforms
Needs applying to the RISC-V class of the same name

Fixes #230

...m.core/src/org/eclipse/embedcdt/managedbuild/cross/arm/core/EnvironmentVariableSupplier.java

ilg-ul · 2023-06-21T18:38:03Z

Should we be concerned with compatibility issues? My first impulse is to leave the buggy code there, with a deprecation notice, and add a new class with the fix.

jonahgraham · 2023-06-21T19:51:21Z

Should we be concerned with compatibility issues? My first impulse is to leave the buggy code there, with a deprecation notice, and add a new class with the fix.

I'll have a think about that more while I write and test it. The external behaviour of the code isn't changing with my fix, except for places where the entries are being corrupted. Therefore my current instinct is to simply fix the issue.

ilg-ul · 2023-06-21T19:59:52Z

If in the end only the private class or private members are changed, you are right, we should have no compatibility issues.

ilg-ul · 2023-06-27T15:56:46Z

Do you plan more work on this PR?

jonahgraham · 2023-06-27T16:05:48Z

Do you plan more work on this PR?

Yes - I will finish this up in the coming days.

ilg-ul · 2023-06-27T18:17:19Z

...core/src/org/eclipse/embedcdt/managedbuild/cross/riscv/core/EnvironmentVariableSupplier.java

-
-				path = xpackBinPath.toOSString();
+				String xpackBinPath = project.getFolder("xpacks").getFolder(".bin").getLocation().toOSString();
+				path.add(xpackBinPath);


Does this produce the absolute path of the xpacks/.bin folder located inside the project?

This fixed a bug that the code assumed that the project was located on disk in the workspace folder.

e.g. if I have a project on my computer in E:\cdt\git\armproj and my workspace is E:\cdt\runtime-CDT then the old code would add E:\cdt\runtime-CDT\armproj\xpacks\.bin to the path variable, when it should have added E:\cdt\git\armproj\xpacks\.bin

This was happening because of the order of resolution. The updated code gets the full internal path to the bin first (e.g. project.getFolder("xpacks").getFolder(".bin") == F/armproj/xpacks/.bin) and then gets the location on disk of that folder. The old code (project.getWorkspace().getRoot().getLocation()) got the location on disk of the workspace and then appended paths to it.

Let me know if you want me to split that out to a different PR.

Does this produce the absolute path of the xpacks/.bin folder located inside the project?

TL;DR - Yes! :)

jonahgraham · 2023-06-27T18:36:01Z

I have tested this on Linux and Windows. On Windows since I don't actually have a server to install tools I used \\machine\c$. So instead of:

C:/Users/ditto-win/AppData/Roaming/xPacks/@xpack-dev-tools/windows-build-tools/4.4.0-1.1/.content/bin

I used:

\\ditto-win\c$\Users\ditto-win\AppData\Roaming\xPacks\@xpack-dev-tools\windows-build-tools\4.4.0-1.1\.content\bin

(ditto-win is both the name of the Windows computer I am using and the user name)

ilg-ul · 2023-06-27T18:53:48Z

In your example you show the path for the windows-build-tools, which is retrieved from the preferences as an absolute path.

My question refers to the xpacks/.bin folder. Do you see the absolute path of this folder in the generated PATH?

This is a relatively recent feature, unfortunately not very well documented. :-(

xpm can also install toolchains inside a project, in the xpacks/.bin folder, and in this case the local toolchain is preferred, so all other settings in the MCU preferences/properties, are ignored.

The advantage of such a configuration is that the versions of the binary tools are stored in the package.json, and exactly the same versions can be later installed if the project is cloned on a separate machine, thus being one step closer to reproducible builds.

jonahgraham · 2023-06-27T19:01:06Z

In your example you show the path for the windows-build-tools, which is retrieved from the preferences as an absolute path.

This comment #576 (comment) was not in reply to your question in #576 (comment) - please see #576 (comment)

My question refers to the xpacks/.bin folder. Do you see the absolute path of this folder in the generated PATH?

Yes.

This is a relatively recent feature, unfortunately not very well documented. :-(

AFAICT the feature currently only works if the project is checked out into the workspace folder. Also the normal paths are added to the path to.

e.g. if you have a project with xpacks/.bin in your project outside of the workspace the PATH gets an invalid location for xpacks/.bin and then get the global (or workspace) path to the toolchain. So it looks like it works, but it is not using gcc in the project's xpacks/.bin.

When preferXpacksBin is true should it stop adding other locations to the PATH?

ilg-ul · 2023-06-27T19:17:27Z

AFAICT the feature currently only works if the project is checked out into the workspace folder.

Hmmm... not good... how can we make it work for standalone projects?

That's the point with the xpm dependencies, they are local to the project, regardless where the project is located.

ilg-ul · 2023-06-27T19:23:40Z

When preferXpacksBin is true should it stop adding other locations to the PATH?

Not only that it should stop adding other locations to the PATH (except the windows build tools), but the xpacks/.bin must be guaranteed to be the very first folder in the PATH. This is how xpm/npm provide reproducible builds, and Eclipse should implement it too.

jonahgraham · 2023-06-27T19:32:34Z

AFAICT the feature currently only works if the project is checked out into the workspace folder.

Hmmm... not good... how can we make it work for standalone projects?

That is what the change I introduced fixed. It will work once you merge.

When preferXpacksBin is true should it stop adding other locations to the PATH?

Not only that it should stop adding other locations to the PATH (except the windows build tools), but the xpacks/.bin must be guaranteed to be the very first folder in the PATH. This is how xpm/npm provide reproducible builds, and Eclipse should implement it too.

In that case I will wrap the rest of the path code in the else block of if (preferXpacksBin) { - new PR on its way in a moment.

ilg-ul · 2023-06-27T19:40:27Z

I will wrap the rest of the path code in the else block of if (preferXpacksBin)

be sure you keep the windows build tools, if defined, and locate them deeper than xpacks/.bin.

jonahgraham · 2023-06-27T20:29:22Z

I'm going to split the xpacks into a new PR.

The old code placed multiple entries for adding to PATH in a File temporarily to get the absolute path. This worked generally on non-Windows platforms because often there was only a single entry. However on Windows there are generally two entries. Adding to that is the UNC path names that are sometimes present on Windows and the temporary use of File causes corruption in the settings. Instead collect the new PATH entries in a list of strings, and then before making the final PATH setting, run each individual entry through new File(entry).getAbsolutePath() Fixes eclipse-embed-cdt#230

jonahgraham · 2023-06-27T20:36:22Z

I have split out any of the changes related to preferXpacksBin as it was clouding the original purpose of this PR which was to fix #230.

@ilg-ul this PR is ready to go now and I will soon (but may not be today) provide a new PR for preferXpacksBin so we can discuss that.

ilg-ul · 2023-06-28T10:56:56Z

I entered #580 for the changes related to preferXpacksBin.

jonahgraham mentioned this pull request Jun 21, 2023

Multiple leading backslashes of Windows network path incorrectly removed from preference for toolchain folder #230

Closed

jonahgraham commented Jun 21, 2023

View reviewed changes

...m.core/src/org/eclipse/embedcdt/managedbuild/cross/arm/core/EnvironmentVariableSupplier.java Outdated Show resolved Hide resolved

jonahgraham force-pushed the fix-unc-paths branch from 3b6ec14 to 3f709b1 Compare June 27, 2023 17:45

jonahgraham linked an issue Jun 27, 2023 that may be closed by this pull request

Multiple leading backslashes of Windows network path incorrectly removed from preference for toolchain folder #230

Closed

jonahgraham marked this pull request as ready for review June 27, 2023 18:10

ilg-ul reviewed Jun 27, 2023

View reviewed changes

jonahgraham force-pushed the fix-unc-paths branch from 3f709b1 to 7c4b9ae Compare June 27, 2023 20:34

jonahgraham requested a review from ilg-ul June 27, 2023 20:36

ilg-ul approved these changes Jun 28, 2023

View reviewed changes

ilg-ul merged commit 5f5a527 into eclipse-embed-cdt:develop Jun 28, 2023
2 checks passed

ilg-ul mentioned this pull request Jun 28, 2023

Review the logic for setting the PATH with preferXpacksBin #580

Closed

ilg-ul mentioned this pull request Jul 3, 2023

Global RISC-V Toolchains name does not change with dropdown box #574

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework setting PATH variable to not store in a File temporarily #576

Rework setting PATH variable to not store in a File temporarily #576

jonahgraham commented Jun 21, 2023 •

edited

ilg-ul commented Jun 21, 2023

jonahgraham commented Jun 21, 2023

ilg-ul commented Jun 21, 2023

ilg-ul commented Jun 27, 2023

jonahgraham commented Jun 27, 2023

ilg-ul Jun 27, 2023

jonahgraham Jun 27, 2023

jonahgraham Jun 27, 2023

jonahgraham commented Jun 27, 2023

ilg-ul commented Jun 27, 2023

jonahgraham commented Jun 27, 2023

ilg-ul commented Jun 27, 2023

ilg-ul commented Jun 27, 2023

jonahgraham commented Jun 27, 2023

ilg-ul commented Jun 27, 2023 •

edited

jonahgraham commented Jun 27, 2023

jonahgraham commented Jun 27, 2023

ilg-ul commented Jun 28, 2023

Rework setting PATH variable to not store in a File temporarily #576

Rework setting PATH variable to not store in a File temporarily #576

Conversation

jonahgraham commented Jun 21, 2023 • edited

ilg-ul commented Jun 21, 2023

jonahgraham commented Jun 21, 2023

ilg-ul commented Jun 21, 2023

ilg-ul commented Jun 27, 2023

jonahgraham commented Jun 27, 2023

ilg-ul Jun 27, 2023

Choose a reason for hiding this comment

jonahgraham Jun 27, 2023

Choose a reason for hiding this comment

jonahgraham Jun 27, 2023

Choose a reason for hiding this comment

jonahgraham commented Jun 27, 2023

ilg-ul commented Jun 27, 2023

jonahgraham commented Jun 27, 2023

ilg-ul commented Jun 27, 2023

ilg-ul commented Jun 27, 2023

jonahgraham commented Jun 27, 2023

ilg-ul commented Jun 27, 2023 • edited

jonahgraham commented Jun 27, 2023

jonahgraham commented Jun 27, 2023

ilg-ul commented Jun 28, 2023

jonahgraham commented Jun 21, 2023 •

edited

ilg-ul commented Jun 27, 2023 •

edited