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

Adds a new --std-path argument to set standard library path. #11778

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

tritao
Copy link
Contributor

@tritao tritao commented Sep 21, 2024

This PR adds an --std-path argument to set standard library path as an alternative to setting the HAXE_STD_PATH environment variable.

This is useful to be able to use Haxe from the ninja build system, as it does not support setting environment variables from its build scripts: ninja-build/ninja#1002

I have also read that I might need to set HAXELIB_PATH and HAXEPATH to get haxelib working. I quickly checked the source code and the compiler does seem to invoke haxelib in some places, so would it be acceptable to add more flags for those paths? Or is there a better mechanism that could be used as an alternative?

Also this is my first using OCaml and hacking on the compiler to let me know of any issues with the code.

@tritao
Copy link
Contributor Author

tritao commented Sep 21, 2024

Ok, I think some weirdness is happening with the CI.

The Build Haxe build step is green and reported as passing, but its actually failing:

File "src/optimization/dce.ml", line 90, characters 13-48:
90 | 	List.exists (ExtString.String.starts_with file) dce.std_dirs
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: This expression has type prefix:string -> bool
       but an expression was expected of type 'a -> bool
make: *** [Makefile:81: haxe] Error 1

... and its failing with the error that my other PR fixes: #11779

Some quick search shows that it may need $ErrorActionPreference = 'Stop' there to stop if any command fails.

This PR adds an `--std-path` argument to set standard library path as an
alternative to setting the `HAXE_STD_PATH` environment variable.

This is useful to be able to use Haxe from the ninja build system,
as it does not support setting environment variables from its build
scripts: ninja-build/ninja#1002
@Simn
Copy link
Member

Simn commented Sep 22, 2024

Two things:

  1. We try to avoid magic values like "", so it's better to declare com.std_path as string option and then do (match com.std_path with None -> (* the "" case *) | Some std_path -> (* the other case *)).

  2. This hooks into get_std_class_paths, which is called via setup_common_context, which is called before parse_args (see compile_ctx in compiler.ml). Unless I'm misreading something here I can't see how this implementation actually works timing-wise because com.std_path is set only after it's relevant.

@skial skial mentioned this pull request Sep 22, 2024
1 task
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.

2 participants