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

make use of cargo workspace for tests #243

Closed
wants to merge 2 commits into from

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Sep 12, 2022

There is no need to run cargo test/clippy/fmt for each crate individually in the workspace. Reusing the workspace target directory that Corrosion uses drastically increases the speed of the tests by reusing compilation artifacts from the main build.

Depends on #229 to avoid merge conflicts

@Be-ing Be-ing marked this pull request as draft September 12, 2022 20:31
@Be-ing Be-ing force-pushed the workspace_tests branch 3 times, most recently from 1ac9c07 to e3b8fcf Compare September 13, 2022 14:06
@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 13, 2022

:/ Windows is failing to link the tests...

  = note: cxx-qt-gen.lib(ffi.cxx.o) : error LNK2019: unresolved external symbol get_cpp_number referenced in function cxxbridge1$get_cpp_number
          C:\cxx-qt\build\cargo\build\debug\deps\basic_cxx_only-c76a0ebb8efe9746.exe : fatal error LNK1120: 1 unresolved externals

@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 15, 2022

That link error already exists in the main branch, but it was obscured by excluding most of the tests on Windows.

@ahayzen-kdab
Copy link
Collaborator

This was implemented in another change.

@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 28, 2022

It was? What change? CMake is still calling cargo test per crate on the main branch.

@ahayzen-kdab
Copy link
Collaborator

Oh lol, sorry i thought this was my workspace PR which put all the examples in the same workspace :')

@ahayzen-kdab ahayzen-kdab reopened this Sep 28, 2022
This is much faster because it avoids redundant compilation.
@OlivierLDff
Copy link
Contributor

I will have a look at that if you want ;)

@Be-ing
Copy link
Contributor Author

Be-ing commented Apr 1, 2023

@OlivierLDff that would be great :)

@OlivierLDff OlivierLDff mentioned this pull request Apr 2, 2023
@Be-ing
Copy link
Contributor Author

Be-ing commented Apr 2, 2023

Superseded by #509

@Be-ing Be-ing closed this Apr 2, 2023
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.

3 participants