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

Switch from Rust nightly to Rust stable #1218

Merged
merged 11 commits into from
Jul 29, 2022
Merged

Switch from Rust nightly to Rust stable #1218

merged 11 commits into from
Jul 29, 2022

Conversation

jhaye
Copy link
Collaborator

@jhaye jhaye commented Jun 10, 2022

The only reason remaining why we use Rust nightly is because only on this release channel cargo supports setting an output dir. This was previously discussed here: https://github.com/lf-lang/reactor-rust/issues/15

I have implemented the functionality in Kotlin code by copying the generated binary to the designated output dir. This allows us to get rid of the Rust nightly requirement.

jhaye added 2 commits June 10, 2022 13:07
This retains functionality that for now is only available via Rust Nightly
@jhaye jhaye added the rust Related to the Rust target label Jun 10, 2022
@jhaye jhaye requested a review from cmnrd June 10, 2022 11:16
Copy link
Collaborator

@cmnrd cmnrd left a comment

Choose a reason for hiding this comment

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

Looks great! Do you have an idea why the rust tests fail though? We have a lot of errors like this: "/home/runner/work/lingua-franca/lingua-franca/test/Rust/src-gen/ReactionLabels/target/debug/reaction_labels: The source file doesn't exist." in the log.

@jhaye
Copy link
Collaborator Author

jhaye commented Jul 22, 2022

I have no clue why this is failing. Looking at how the code generation is reported, it should be building the binaries at exactly the place where it reports no file exists.

@jhaye
Copy link
Collaborator Author

jhaye commented Jul 27, 2022

Another possibility that I have eliminated: I thought it might be that this is because CI jobs are run with Rust Nightly exclusively, but with Nightly as default it still works on my machine.

@cmnrd
Copy link
Collaborator

cmnrd commented Jul 28, 2022

On my end also everything works fine with both the stable and nightly toolchains. I am not sure what is happening here and its a pain to debug these CI jobs. I guess our best shot is to add a few ls commands to see what the actual directory structure is. I will give this a shot

@cmnrd cmnrd force-pushed the rust-stable branch 2 times, most recently from d9bd230 to 219adbc Compare July 28, 2022 12:09
@jhaye
Copy link
Collaborator Author

jhaye commented Jul 28, 2022

Looking at the Windows tests failing, I hadn't considered that on Windows compilation produces .EXE files.

@cmnrd
Copy link
Collaborator

cmnrd commented Jul 28, 2022

Ah, ok so that was the issue with Windows. OK :)

So I found that somehow the checked in Cargo.toml in src-gen broke the compilation setup. I don't fully understand why, but removing it fixes the problem. I could actually reproduce the original problem locally when the Cargo.toml is checked out.
Removing the Cargo.toml indeed increases the CI run time, but I think it is still Ok.

I think if we want to use the top-level Cargo.toml, we should probably code generate it and find a general solution.

@jhaye
Copy link
Collaborator Author

jhaye commented Jul 28, 2022

Okay, I'll keep that in mind. Thank you for debugging! I will fix up this PR tomorrow so that we can finally merge it.

@cmnrd
Copy link
Collaborator

cmnrd commented Jul 29, 2022

Looks like the Windows job runs out of disk space... I guess we will have to fix the Cargo.toml that I deleted so that reactor-rs is not build X times. But I don't know where the problem lies with the original Cargo.toml

@jhaye
Copy link
Collaborator Author

jhaye commented Jul 29, 2022

I think I understand what the problem is here, the root of a workspace is usually where the target directory is put when building.

As it says in the Cargo book:

All packages share a common output directory, which defaults to a directory named target in the workspace root.

I have an idea how to fix this issue. Cargo exposes several environment variables when building, which can also be got via cargo metadata. The RustValidator class already does this to some degree, I would just have to expose it via API and add the CARGO_TARGET_DIR to it and then retrieve the target directory through that.

@jhaye
Copy link
Collaborator Author

jhaye commented Jul 29, 2022

The Rust tests are passing now on every platform.

Copy link
Collaborator

@cmnrd cmnrd left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for fixing everything!

@cmnrd cmnrd merged commit be71d83 into master Jul 29, 2022
@cmnrd cmnrd deleted the rust-stable branch July 29, 2022 13:37
@jhaye jhaye mentioned this pull request Aug 24, 2022
@lhstrh lhstrh changed the title Compile Rust projects with Rust stable Switch from Rust nightly to Rust stable Jan 26, 2023
@lhstrh lhstrh added the refactoring Code quality enhancement label Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Code quality enhancement rust Related to the Rust target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants