HaxeFoundation / haxe

Haxe - The Cross-Platform Toolkit
https://haxe.org
6.17k stars 654 forks source link

--neko-lib and haxelib ndll behaviour #10997

Closed tobil4sk closed 1 year ago

tobil4sk commented 1 year ago

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: https://github.com/HaxeFoundation/haxelib/commit/7c8a5938d12b895b59a98922149eedaf16a7a456. 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 commented 1 year ago

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

tobil4sk commented 1 year ago

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:

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: https://github.com/HaxeFoundation/haxelib/issues/509

Simn commented 1 year ago
  • Remove the built-in haxelib resolution code, since it is unused anyway

What code are you referring to here?

tobil4sk commented 1 year ago

What code are you referring to here?

https://github.com/HaxeFoundation/haxe/blob/5b6b76ea2d17bb2b8e061622483d35093e8e2dbe/src/generators/genneko.ml#L700-L712

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

tobil4sk commented 1 year ago

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.