Try building FFI in CI, including Windows #17
Conversation
|
It doesn't seem to be reusing the cache from the earlier cargo builds, so that's either something to look into or we should split it into a separate action so it starts earlier, as this step takes a bit of time. |
|
Looks good to me! |
I think additional cache key needs to be added- since build-ffi uses --release build |
|
@pawelchcki I split the workflow; can you add the cache key? I need to work on some other stuff for a bit. |
Will do |
|
I've handed this PR over to the .NET profiling people to get that windows stuff working. |
Sorry for the delay, I'm taking this. |
gleocadie
force-pushed
the
levi/ffi-ci
branch
3 times, most recently
from
c8a07a5
to
7d53a90
Compare
gleocadie
force-pushed
the
levi/ffi-ci
branch
6 times, most recently
from
aab435a
to
ee43b33
Compare
gleocadie
force-pushed
the
levi/ffi-ci
branch
7 times, most recently
from
8571b62
to
ab26950
Compare
gleocadie
force-pushed
the
levi/ffi-ci
branch
3 times, most recently
from
2a4c9a2
to
24f5b35
Compare
.github/workflows/test.yml
Outdated
| @@ -2,18 +2,19 @@ name: Test | |||
| on: [push] | |||
| env: | |||
| CARGO_TERM_COLOR: always | |||
| RUST_VERSION: 1.56.1 | |||
There was a problem hiding this comment.
While this PR was open, it seems main upgraded to 1.60 and this PR brings it back down (probably not on purpose). Looking through the commits, we may need to merge in main?
There was a problem hiding this comment.
maybe updating from main is the thing to do.
| expected_native_static_libs="" # I don't know what to expect | ||
| native_static_libs="" # I don't know what to expect |
There was a problem hiding this comment.
Hmm. How did we not end up with test failures with these set to blank?
There was a problem hiding this comment.
hum good question, maybe because cargo was not able to retrieve those informations
There was a problem hiding this comment.
I think the libs NtDll;UserEnv;Bcrypt;crypt32;wsock32;ws2_32;shlwapi should be in this list, rather than hard-coding them in the CMakeLists.txt. That's how it works on other platforms. That is, if cargo is working "correctly" and knows about the deps (it should).
There was a problem hiding this comment.
but how would you compile using the CMake if you do not specify the list. (sorry new to that)
If the CMakeList is generated why pushing it ?
| with: | ||
| path: | | ||
| ~/.cargo/registry/ | ||
| ~/.cargo/git/db/ | ||
| ~/.cargo/bin/ | ||
| target/ | ||
| key: v1-${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.toml') }}-${{ inputs.rust_version }} | ||
| key: v1-${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.toml') }}-${{ inputs.rust_version }}-${{ inputs.build_profile }} |
There was a problem hiding this comment.
should we consider the lock file ?
There was a problem hiding this comment.
I think since we commit Cargo.lock anyway that it doesn't matter. If we update a version, then it won't be in the cache and it will pull it.
|
@pawelchcki we will need your approval on this one (core things) |
What does this PR do?
Builds the FFI artifacts on all platforms, including Windows.
Motivation
The FFI has been broken several times in the past because this wasn't being run in CI.
Additional Notes
I don't know much about Windows, I may need help fixing everything.
How to test the change?
No changes to test; the CI covers everything.