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

Sync IR changes #225

Merged
merged 192 commits into from
Oct 3, 2024
Merged

Sync IR changes #225

merged 192 commits into from
Oct 3, 2024

Conversation

waterlens
Copy link
Member

No description provided.

@LPTK
Copy link
Contributor

LPTK commented Aug 3, 2024

If the compiler project's tests now need to run only from the nix command (which I don't like), you should change the build.sbt so compilerJVM is not part of the .aggregate root project.

But I don't like this very much. Can't we just run something like "nix install" and then make sure the correct tools are used when calling them from SBT?

# Conflicts:
#	compiler/shared/test/diff-ir/Class.mls
#	compiler/shared/test/diff-ir/Currying.mls
#	compiler/shared/test/diff-ir/LiftClass.mls
#	compiler/shared/test/scala/mlscript/compiler/TestIR.scala
@waterlens
Copy link
Member Author

waterlens commented Aug 4, 2024

If the compiler project's tests now need to run only from the nix command (which I don't like), you should change the build.sbt so compilerJVM is not part of the .aggregate root project.

But I don't like this very much. Can't we just run something like "nix install" and then make sure the correct tools are used when calling them from SBT?

I think Nix never changes things outside the environment, rather they have overlays where you have your settings and then offer you commands to go inside to use it.

It could be quite slow to invoke nix like using make in DiffTests, because we need to do that for every single runCpp test. If we want that, it would also require us to write the intermediate cpp code into a file in the source tree, otherwise, I don't know how to put it into the nix env ...

@pca006132
Copy link
Member

You can treat nix develop as setting up a docker/vm, and it has the environment variables set inside it.

Technically you can install things globally, but it is not idiomatic and kind of violates the nix philosophy.

@LPTK
Copy link
Contributor

LPTK commented Aug 5, 2024

@pca006132 Could you please describe the full solution that using nix develop implies?

  1. What are the commands, if any, that the CI and local users need to run in order to install the tools? (Of course, no global installs!)
  2. What are the commands that the SBT process is supposed to shell out in order to run the C++ compiler?

@LPTK
Copy link
Contributor

LPTK commented Aug 5, 2024

It's probably fine if we have to use the SBT command from some sort of special command like <setup nix env> sbt test

@pca006132
Copy link
Member

CI: nix develop --command sbt compilerJVM/test.

Local user:

# enter shell
nix develop
# inside the shell...
sbt compilerJVM/test

I typically use direnv. If you use it with the file .envrc with use_flake in the directory, you can just enter the directory and just run sbt compilerJVM/test, the environment is automatically setup when you enter the directory and destroyed when you exit.

@LPTK
Copy link
Contributor

LPTK commented Aug 5, 2024

@pca006132 Thanks!

@waterlens Does this work for you?

@waterlens
Copy link
Member Author

@pca006132 Thanks!

@waterlens Does this work for you?

I think I have used it in the CI. So, what's the next step? Moving compilerJVM out of the root target?

@LPTK
Copy link
Contributor

LPTK commented Aug 6, 2024

I just checked the CI config. I don't understand why you used nix develop --command sbt compilerJVM/test instead of running everything under the nix environment as nix develop --command sbt test (replacing the old test command).

@waterlens
Copy link
Member Author

waterlens commented Aug 6, 2024

I once thought it was only used to set up the environment for testing cpp backend. Anyway, if you accept the idea to test the whole project in a nix environment, I will change it accordingly.

@LPTK
Copy link
Contributor

LPTK commented Aug 6, 2024

Yeah, there's no reason not to place the whole thing under nix. It would also be useful to fix the nodejs version while we're at it!

@waterlens
Copy link
Member Author

waterlens commented Aug 6, 2024

As far as I know, nixpkgs only support LTS version NodeJS. Can we upgrade the version from 17 to 18? Otherwise, I may need to install NodeJs from the tarball.

@LPTK
Copy link
Contributor

LPTK commented Aug 10, 2024

Ok I managed to fix the CI and to make the tests pass on my older local macOS version by switching to g++!

@LPTK LPTK merged commit c389926 into hkust-taco:mlscript Oct 3, 2024
1 check passed
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.

4 participants