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

feat: Enable global expose with nested paths #2362

Merged
merged 45 commits into from
Nov 26, 2024
Merged

Conversation

bahugo
Copy link
Contributor

@bahugo bahugo commented Oct 27, 2024

Closes #2350

I tried to be as explicit as I could, I modified Mapping attribute to executable_relname (relative path but with no extension), and executable_name() is now evaluated based on relname.

@bahugo bahugo changed the title Enable global expose with nested paths feat: Enable global expose with nested paths Oct 27, 2024
@bahugo bahugo marked this pull request as draft October 27, 2024 15:19
@bahugo bahugo marked this pull request as ready for review October 27, 2024 16:24
Copy link
Contributor

@ruben-arts ruben-arts left a comment

Choose a reason for hiding this comment

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

Thanks for the work! I've left some comments but the general implementation looks good.

If you need help creating a dummy package let us know! It should be pretty straight forward to create your situation in a dummy package and test its logic in there.

src/global/mod.rs Outdated Show resolved Hide resolved
src/global/project/manifest.rs Outdated Show resolved Hide resolved
src/global/project/manifest.rs Show resolved Hide resolved
src/global/project/manifest.rs Outdated Show resolved Hide resolved
@bahugo
Copy link
Contributor Author

bahugo commented Oct 28, 2024

@ruben-arts I tried to add an integration test.
I modified dummy-channel-1, to a add a nested executable.
I'm not sure about which directory is $PREFIX/bin is it the bin directory were pixi will store exposed executables?
Anyway, I added rattler-build dependency to pytest feature and ran pixi r update-integration-test-data but it failed:

│ ╭─ Running build script
 │ │ %SRC_DIR%>IF "" == "" (
 │ │
 │ │  call %SRC_DIR%\build_env.bat
 │ │ )
 │ │ La syntaxe de la commande n’est pas correcte.
 │ │ %SRC_DIR%>mkdir -p $PREFIX/bin
 │ │ %SRC_DIR%>echo "dummy_e on windows"  1>$PREFIX/bin/dummy_e.bat
 │ │ Le chemin d’accès spécifié est introuvable.
 │ │ × error Script failed with status 1
 │ │ × error Work directory: 'C:\Users\bahugo\dev\pixi\tests\integration\test_data\channels\dummy_channel_1\bld\rattler-build_dummy_e_1730147400\work'
 │ │ × error To debug the build, run it manually in the work directory (execute the `./conda_build.sh` or `conda_build.bat` script)
 │ │
 │ ╰─────────────────── (took 0 seconds)
 │
 ╰─────────────────── (took 0 seconds)
Error:   × Script failed

Traceback (most recent call last):
  File "C:\Users\bahugo\dev\pixi\tests\integration\test_data\update-channels.py", line 37, in <module>
    main()
  File "C:\Users\bahugo\dev\pixi\tests\integration\test_data\update-channels.py", line 18, in main
    subprocess.run(
  File "C:\Users\bahugo\dev\pixi\.pixi\envs\default\Lib\subprocess.py", line 571, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['rattler-build', 'build', '--target-platform', 'win-64', '--output-dir', 'channels/dummy_channel_1', '--recipe', 'recipes/dummy_channel_1.yaml']' returned non-zero exit status 1.

I tried to run conda_build.bat but it failed too.
I'm not familiar with rattler-build, do you have an idea?

@bahugo
Copy link
Contributor Author

bahugo commented Oct 29, 2024

@ruben-arts I tried to add an integration test. I modified dummy-channel-1, to a add a nested executable. I'm not sure about which directory is $PREFIX/bin is it the bin directory were pixi will store exposed executables? Anyway, I added rattler-build dependency to pytest feature and ran pixi r update-integration-test-data but it failed:

│ ╭─ Running build script
 │ │ %SRC_DIR%>IF "" == "" (
 │ │
 │ │  call %SRC_DIR%\build_env.bat
 │ │ )
 │ │ La syntaxe de la commande n’est pas correcte.
 │ │ %SRC_DIR%>mkdir -p $PREFIX/bin
 │ │ %SRC_DIR%>echo "dummy_e on windows"  1>$PREFIX/bin/dummy_e.bat
 │ │ Le chemin d’accès spécifié est introuvable.
 │ │ × error Script failed with status 1
 │ │ × error Work directory: 'C:\Users\bahugo\dev\pixi\tests\integration\test_data\channels\dummy_channel_1\bld\rattler-build_dummy_e_1730147400\work'
 │ │ × error To debug the build, run it manually in the work directory (execute the `./conda_build.sh` or `conda_build.bat` script)
 │ │
 │ ╰─────────────────── (took 0 seconds)
 │
 ╰─────────────────── (took 0 seconds)
Error:   × Script failed

Traceback (most recent call last):
  File "C:\Users\bahugo\dev\pixi\tests\integration\test_data\update-channels.py", line 37, in <module>
    main()
  File "C:\Users\bahugo\dev\pixi\tests\integration\test_data\update-channels.py", line 18, in main
    subprocess.run(
  File "C:\Users\bahugo\dev\pixi\.pixi\envs\default\Lib\subprocess.py", line 571, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['rattler-build', 'build', '--target-platform', 'win-64', '--output-dir', 'channels/dummy_channel_1', '--recipe', 'recipes/dummy_channel_1.yaml']' returned non-zero exit status 1.

I tried to run conda_build.bat but it failed too. I'm not familiar with rattler-build, do you have an idea?

@ruben-arts I've managed to update integration test channel by compiling them use debian in wsl.

I ran integration tests on windows, the test I added ran successfully, but I got a fail for another test:

E           AssertionError: Return code was 106, expected 0, stderr: No pyvenv.cfg file

tests\integration\common.py:55: AssertionError

I don't get why it failed.

```
you can also omit the extension
```
pixi global install dotnet --expose dotnet=dotnet\dotnet
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried this example on Linux but that package doesn't seem to work. Do you understand why the dotnet setup is using a non bin/xx setup for it's binaries?

Copy link
Contributor Author

@bahugo bahugo Oct 30, 2024

Choose a reason for hiding this comment

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

I have no Idea.
I had never tried to install dotnet on linux but you're right, it's installed in .pixi/envs/dotnet/lib/dotnet/dotnet
For some reason everything is painful with windows tools...
Maybe a more cross platform or generic tool would be better for the documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I contacted @dhirschfeld as he is a pixi user and the maintainer of dotnet maybe he has some smart things to say about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about smart - the TL;DR is that it's just a binary repackage of upstream and that seems to be the way it's packaged by Microsoft.

The package sets a number of env vars in an activation script, including the PATH. I'd like to port setting/unsetting the env vars to the JSON format (assuming that works with pixi?) but haven't gotten around to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes pixi supports that 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there another example package, where this would be required, that does work on linux?

@ruben-arts
Copy link
Contributor

ruben-arts commented Oct 30, 2024

While reviewing I checked it out and did some small changes, here is the diff patch you could apply:

diff --git a/src/global/project/manifest.rs b/src/global/project/manifest.rs
index cac1fa50..ecea5f4e 100644
--- a/src/global/project/manifest.rs
+++ b/src/global/project/manifest.rs
@@ -432,6 +432,8 @@ impl Manifest {
 #[derive(Debug, Clone, Serialize, Deserialize, Hash, PartialEq, Eq)]
 pub struct Mapping {
     exposed_name: ExposedName,
+    // The executable_relname is a executable name possibly with a parts of a path in it to match on.
+    // e.g. `dotnet/dotnet` will find `$PREFIX/lib/dotnet/dotnet`
     executable_relname: String,
 }
 
@@ -450,13 +452,14 @@ impl Mapping {
     pub fn executable_relname(&self) -> &str {
         &self.executable_relname
     }
+
+    // Splitting the executable_relname by the last '/' and taking the last part
+    // e.g. 'nested/test_executable' -> 'test_executable'
     pub fn executable_name(&self) -> &str {
-        if let Some(executable_file_name) = Path::new(&self.executable_relname).file_name() {
-            return executable_file_name
-                .to_str()
-                .unwrap_or(&self.executable_relname);
-        };
-        &self.executable_relname
+        Path::new(&self.executable_relname)
+            .file_name()
+            .and_then(|name| name.to_str())
+            .unwrap_or(&self.executable_relname)
     }
 }
 
diff --git a/tests/integration/test_global.py b/tests/integration/test_global.py
index a5ab777e..93471054 100644
--- a/tests/integration/test_global.py
+++ b/tests/integration/test_global.py
@@ -308,12 +308,6 @@ def test_expose_basic(pixi: Path, tmp_path: Path, dummy_channel_1: str) -> None:
     )
     assert not dummy1.is_file()
     assert not dummy3.is_file()
-    # extension = "exe" if os.name == "nt" else "sh"
-    # # Add nested dummy1
-    # verify_cli_command(
-    #     [pixi, "global", "expose", f"nested_dummy=nested/dummy.{extension}"],
-    #     env=env,
-    # )
 
 
 def test_expose_revert_working(pixi: Path, tmp_path: Path, dummy_channel_1: str) -> None:
(END)
 
diff --git a/tests/integration/test_global.py b/tests/integration/test_global.py
index a5ab777e..93471054 100644
--- a/tests/integration/test_global.py
+++ b/tests/integration/test_global.py
@@ -308,12 +308,6 @@ def test_expose_basic(pixi: Path, tmp_path: Path, dummy_channel_1: str) -> None:
     )
     assert not dummy1.is_file()
     assert not dummy3.is_file()
-    # extension = "exe" if os.name == "nt" else "sh"
-    # # Add nested dummy1
-    # verify_cli_command(
-    #     [pixi, "global", "expose", f"nested_dummy=nested/dummy.{extension}"],
-    #     env=env,
-    # )
 
 
 def test_expose_revert_working(pixi: Path, tmp_path: Path, dummy_channel_1: str) -> None:

@ruben-arts ruben-arts closed this Oct 30, 2024
@ruben-arts
Copy link
Contributor

ruben-arts commented Oct 30, 2024

My browser bugged out and closed this issue by accident.

@ruben-arts ruben-arts reopened this Oct 30, 2024
@bahugo bahugo force-pushed the main branch 2 times, most recently from 2d24894 to b2b117d Compare November 4, 2024 08:57
@bahugo
Copy link
Contributor Author

bahugo commented Nov 4, 2024

Something is not working with python activation in integration test, I don't know why...

  Windows 10.0.26100 on D049 on  main +/- is 📦 v0.34.0 🦀 v1.81.0
~\dev\pixi ➜ C:\Users\bahugo\dev\pixi\pytest-temp\popen-gw0\test_sync_dependencies0\bin\python-injected.bat --version
> No pyvenv.cfg file

  Windows 10.0.26100 on D049 on  main +/- is 📦 v0.34.0 🦀 v1.81.0
~\dev\pixi ❯ C:\Users\bahugo\dev\pixi\pytest-temp\popen-gw0\test_sync_dependencies0\envs\test\Lib\venv\scripts\nt\python.exe
> No pyvenv.cfg file

@ruben-arts
Copy link
Contributor

I'll give this a go today! See if I can fix the issues

@ruben-arts
Copy link
Contributor

Hey I fixed one more issue and cleanup the merge.

The issue I worked on was the fact that when using the nested path as the name it would put it in .pixi/bin/nested/path/xxx.exe.

Copy link
Contributor

@Hofer-Julian Hofer-Julian left a comment

Choose a reason for hiding this comment

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

Thanks, @bahugo and @ruben-arts, great work!

@Hofer-Julian Hofer-Julian enabled auto-merge (squash) November 18, 2024 15:50
@ruben-arts
Copy link
Contributor

There is still an issue with this logic, the pixi global install python now exposes a different version of python.exe as there are two in the whole binary. We need to get rid of the nested logic if the expose name is not a path like object.

@bahugo
Copy link
Contributor Author

bahugo commented Nov 20, 2024

There is still an issue with this logic, the pixi global install python now exposes a different version of python.exe as there are two in the whole binary. We need to get rid of the nested logic if the expose name is not a path like object.

@ruben-arts I don't think that nested path is the reason why it fails for python on windows.
In global/mod.rs I've changed executable search to find every executables like in every folder to be able to expose them if needed.
I think I must add a filter for cases where 2 executables where found, in those cases I must keep the one located in one of those folders :

let binary_folders = if cfg!(windows) {
        &([
            "",
            "Library/mingw-w64/bin/",
            "Library/usr/bin/",
            "Library/bin/",
            "Scripts/",
            "bin/",
        ][..])
    } else {
        &(["bin"][..])
    };

I'll give it a try tomorrow.

auto-merge was automatically disabled November 24, 2024 17:22

Head branch was pushed to by a user without write access

@bahugo
Copy link
Contributor Author

bahugo commented Nov 25, 2024

@ruben-arts @Hofer-Julian it's finally working!

Copy link
Contributor

@Hofer-Julian Hofer-Julian left a comment

Choose a reason for hiding this comment

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

Thanks @bahugo!

@Hofer-Julian Hofer-Julian merged commit d90529d into prefix-dev:main Nov 26, 2024
43 checks passed
jjjermiah pushed a commit to jjjermiah/pixi that referenced this pull request Nov 30, 2024
Closes prefix-dev#2350

I tried to be as explicit as I could, I modified Mapping attribute to
executable_relname (relative path but with no extension), and
executable_name() is now evaluated based on relname.

---------

Co-authored-by: Ruben Arts <[email protected]>
Co-authored-by: Hofer-Julian <[email protected]>
Co-authored-by: Julian Hofer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pixi global exposed doesn't work with nested paths
4 participants