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

--neko-lib and haxelib ndll behaviour #10997

Closed
tobil4sk opened this issue Mar 3, 2023 · 8 comments · Fixed by #11056
Closed

--neko-lib and haxelib ndll behaviour #10997

tobil4sk opened this issue Mar 3, 2023 · 8 comments · Fixed by #11056
Assignees
Milestone

Comments

@tobil4sk
Copy link
Member

tobil4sk commented Mar 3, 2023

While working on #10996, I discovered the oddity that is the behaviour of Haxe's injection of built-in ndll paths for Neko programs.

The --neko-lib flag is new: #10654. First of all, it should probably be renamed to --neko-lib-path or something, because it adds paths to search for haxelibs' ndlls in, it doesn't define or import any actual libraries. We haven't had a release with this flag so it shouldn't be a problem to rename it, and it is mainly used internally anyway (and should be).

My main concern is with the behaviour of adding ndll paths, which long predates this flag. Years ago, Haxelib used to return an ndll path relative to the Haxelib repo, for example: -L lime/8,0,0/ndll/. And so, Haxe would inject haxelib repository resolution code into Neko programs, so they can resolve the full ndll path (e.g. /usr/share/haxe/lib/lime/8,0,0/ndll/).

This was changed in Haxelib (possibly accidentally?) years ago in: HaxeFoundation/haxelib@7c8a593. Now Haxelib returns the absolute path. Hardcoded paths are probably fine for a development build of a program which is unlikely to be used outside of the current environment, although it will break if the Haxelib repository path changes on the system. Outside of development builds, programs should bundle any non-standard ndlls anyway.

The main reason I can think of to use relative ndll paths is Haxelib run.n scripts. For example, if there is a Haxelib script that requires loading some ndll provided by another haxelib, it should be able to load it from the current system's haxelib repository, and this should not be affected by the repository path of the system where the run.n was compiled.

Firstly, we should probably add a compiler flag to turn this behaviour off completely for program builds intended to be distributed. Secondly, I don't really like that this Haxelib resolution behaviour is built into Haxe-generated Neko programs, since it's already been out of date with how Haxelib works for years. This system needs some rethinking (if not for Neko, then for Hashlink or whatever replaces Neko as the default for run scripts). For example, haxelib run ... could set a HAXELIB_PATH variable or something, and then the program could use that to resolve the loader path. This won't work for regular programs though.

@Simn Simn added this to the Release 4.3 milestone Mar 24, 2023
@Simn
Copy link
Member

Simn commented Mar 28, 2023

How about we simply hide this CLI arg? I really only added this for internal usage anyway.

@Simn Simn self-assigned this Mar 28, 2023
@tobil4sk
Copy link
Member Author

tobil4sk commented Mar 28, 2023

If we do haxe-haxec-haxelib and split haxe from haxec, then having this flag will be necessary, so I think it would be a good idea to rename it to something more sensible like --neko-lib-path even we hide from documentation. But once it's hidden, the rest of this issue isn't too urgent, though at some point we should come up with a proper solution for this anyway.

I think we can do this:

  • Remove the built-in haxelib resolution code, since it is unused anyway
  • Add a flag that prevents adding hard-coded ndll paths into the neko program for portable builds which shouldn't have paths that only work on the host system
  • Set an environment variable in haxelib run which neko loads ndlls from, something like NEKO_MODULE_PATH.

This means haxelib run.n files can be compiled without any hard-coded ndll paths, but can load from the current system's haxelib repository so they are fully portable. Also means neko programs are completely unaware of how haxelib works, which is good.

Once we figure out a good system we can implement something similar for hashlink hdlls for when it replaces neko in haxelib:
HaxeFoundation/haxelib#509

@Simn
Copy link
Member

Simn commented Mar 28, 2023

  • Remove the built-in haxelib resolution code, since it is unused anyway

What code are you referring to here?

@tobil4sk
Copy link
Member Author

What code are you referring to here?

"@b", Some (EIf (op "==" es (str p "Windows"),
op "+" (call p (ident p "@env") [str p "HAXEPATH"]) (str p "\\lib\\"),
Some (ETry (
op "+" (call p (loadp "file_contents" 1) [op "+" (call p (ident p "@env") [str p "HOME"]) (str p "/.haxelib")]) (str p "/"),
"e",
(EIf (op "==" es (str p "Linux"),
(EIf (call p (loadp "sys_exists" 1) [ str p "/usr/lib/haxe/lib" ],
str p "/usr/lib/haxe/lib/",
Some (str p "/usr/share/haxe/lib/")),p),
Some (str p "/usr/local/lib/haxe/lib/")
),p)
),p)
),p);

This is added to neko programs by haxe, but is never used because the ndlls paths that get hardcoded are always absolute anyway (and have been since before haxelib 3.3.0).

@Simn
Copy link
Member

Simn commented Mar 28, 2023

Ah right, I'm happy to remove that, and I'm fine with renaming to --neko-lib-path as well. Would that be enough for 4.3?

(As you can probably tell, I'm struggling a bit to understand what problem we're actually solving here.)

@tobil4sk
Copy link
Member Author

Would that be enough for 4.3?

Yes, that should be fine. I can do the removal since I'm somewhat familiar with the code from #10996. We can add the flag for optionally disabling the hard-coded ndll paths at a later point.

The only way this can break anything is if someone is using haxe 4.3.0 together with a haxelib build older than 3.3.0 (I don't think this is even possible, since haxelib used to install itself into haxelib_client before 3.3.0).

(As you can probably tell, I'm struggling a bit to understand what problem we're actually solving here.)

The problems are:

  1. Haxe shouldn't add haxelib resolution code into programs, especially since it is out-dated and unused, as ndll paths are always hard-coded as absolute paths. This will be fixed now by removing the above code.
  2. Hard-coded ndll paths (which are only relevant during development and only work on the host system) should not be added to builds meant to be distributed to other machines. This is why it would be useful to have a flag that turns this off.
  3. A haxelib run script that depends on another haxelib's ndll should be able to load it on any system, but currently it only works on systems with the same haxelib repository path as the original system. Setting an environment variable from haxelib run (which already has access to the haxelib path) it possible for run scripts to be fully portable in this way, without the script itself needing to know anything about haxelib resolution.

@Simn
Copy link
Member

Simn commented Mar 28, 2023

Alright, thanks for the explanation! In that case it's easiest if you open a PR with the changes we need right now.

@tobil4sk
Copy link
Member Author

#11056 should handle everything we need on Haxe's side (points 1 and 2), so once that's merged we can create a separate issue on the haxelib github and close this.

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 a pull request may close this issue.

2 participants