Skip to content

fix: lto in windows, rustc allocator#519

Open
cordx56 wants to merge 79 commits intomainfrom
fix/subset-calc
Open

fix: lto in windows, rustc allocator#519
cordx56 wants to merge 79 commits intomainfrom
fix/subset-calc

Conversation

@cordx56
Copy link
Owner

@cordx56 cordx56 commented Jan 27, 2026

Type Of Change

  • Bug fix
  • New feature
  • Documentation update
  • Enhancement
  • Breaking change
  • Other

@MuntasirSZN
Copy link
Collaborator

Use insta crate pls (as i see you are doing snapshot testing)

@cordx56
Copy link
Owner Author

cordx56 commented Jan 27, 2026

@MuntasirSZN

Use insta crate

I thought insta crate is for unit test. However, this PR implements E2E test.

@cordx56 cordx56 changed the title fix!: Polonius subset calculation algorithm fix!: new Polonius subset calculation algorithm Jan 27, 2026
@MuntasirSZN
Copy link
Collaborator

@MuntasirSZN

Use insta crate

I thought insta crate is for unit test. However, this PR implements E2E test.

So you want to use the final binary and use the show command then check output = snapshot? Well, the part before output can be a function that builds, runs and gives the output, then we use insta to do snapshotting.

Yes its e2e, I am just asking to do it in rust using insta.....

@cordx56
Copy link
Owner Author

cordx56 commented Jan 27, 2026

I don’t know the motivation to use Insta because we may have to change some APIs to test RustOwl using Insta.
But we don’t have to use shell scripts, so I can write it in Rust if it is the best practice.

@MuntasirSZN
Copy link
Collaborator

I don’t know the motivation to use Insta because we may have to change some APIs to test RustOwl using Insta. But we don’t have to use shell scripts, so I can write it in Rust if it is the best practice.

No we dont. Its simple. In rust, we call cargo to build it, then we use that binary to get output, then insta does its thing.

@cordx56
Copy link
Owner Author

cordx56 commented Jan 27, 2026

But our RustOwl API doesn't written for insta E2E test.
And we need rustowlc binary before testing. How can I write the dependency that rustowlc should be built before rustowl test execution?

@MuntasirSZN
Copy link
Collaborator

But our RustOwl API doesn't written for insta E2E test. And we need rustowlc binary before testing. How can I write the dependency that rustowlc should be built before rustowl test execution?

Ummmm a bit of confusion? rustowl api is not written for e2e test? as I said, it will run normal commands, get the output and send to insta macro (one line) get output and send to insta. About build, that test will build rustowl BY INVOKING COMMANDS. (Command::new)

@MuntasirSZN
Copy link
Collaborator

You should look at insta first.

@cordx56
Copy link
Owner Author

cordx56 commented Jan 27, 2026

I already looked insta, and I couldn’t find any command execution macros. I think insta crate is not for E2E test.

@MuntasirSZN
Copy link
Collaborator

We will do command exceution with std (and maybe some other crates) and everything else before checking if it similar to snapshot (insta will do tha).

@cordx56
Copy link
Owner Author

cordx56 commented Jan 28, 2026

Where will we do the command execution? The test requires that rustowl is already built, so it cannot be written in RustOwl code.

@MuntasirSZN
Copy link
Collaborator

Where will we do the command execution? The test requires that rustowl is already built, so it cannot be written in RustOwl code.

Test execution flow:

  • Build rustowl with cargo
  • Then snapshot show whatever.

"ITS NOT RUSTOWL CODE I AM TELLING TO DO WHAT YOU ARE DOING IN THIS PR IN A TEST WITH RUST AND INSTA"

@cordx56
Copy link
Owner Author

cordx56 commented Jan 29, 2026

So should I write test code in another new crate?

@MuntasirSZN
Copy link
Collaborator

So should I write test code in another new crate?

No.

Test macro
Call cargo build with command::new
Then call the debug rustowl
Then use insta to snapshot
Finish

@cordx56
Copy link
Owner Author

cordx56 commented Jan 29, 2026

Inside RustOwl crate?

rustowl and rustowlc should be built before running tests, so should the user build RustOwl before testing?

@MuntasirSZN
Copy link
Collaborator

Uhhhhhhhhh let me do it myself......

@cordx56
Copy link
Owner Author

cordx56 commented Jan 29, 2026

Okay, I will review it.

@MuntasirSZN
Copy link
Collaborator

@cordx56 done e3dc272

@MuntasirSZN
Copy link
Collaborator

I stop now. Lto in windows is not possible, even by rust.

rust-lang/rust#103595

No cross lang lto, as we need an EXACT LLVM toolchain (which is another hard task).

We keep on using normal no lto in windows.

MuntasirSZN
MuntasirSZN previously approved these changes Feb 8, 2026
@MuntasirSZN
Copy link
Collaborator

@cordx56 now review and merge

MuntasirSZN
MuntasirSZN previously approved these changes Feb 8, 2026
MuntasirSZN
MuntasirSZN previously approved these changes Feb 8, 2026
@MuntasirSZN MuntasirSZN added do-build-check Execute build check on PR and removed do-build-check Execute build check on PR labels Feb 8, 2026
@MuntasirSZN
Copy link
Collaborator

@cordx56 Now I am lost. @cordx56

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-build-check Execute build check on PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants