Skip to content
This repository has been archived by the owner on Nov 1, 2023. It is now read-only.

Migrate from winapi to windows-rs #3050

Merged
merged 28 commits into from Aug 8, 2023
Merged

Migrate from winapi to windows-rs #3050

merged 28 commits into from Aug 8, 2023

Conversation

Porges
Copy link
Member

@Porges Porges commented Apr 20, 2023

windows-rs is the newer, Microsoft-supported version of the API bindings; winapi hasn't been updated in some time. This allows us to remove some code, as windows-rs includes the Sym* functions that we had to previously defined ourselves in dbghelp.

Rather than port the jobs and com code I removed it, as it was unused.

The check_hr and check_winapi macros have been replaced by use of .ok()? on BOOL and ? on HRESULT which now support standard error handling facilities.

@codecov-commenter
Copy link

codecov-commenter commented Apr 20, 2023

Codecov Report

Merging #3050 (9562975) into main (3851ab0) will decrease coverage by 1.74%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3050      +/-   ##
==========================================
- Coverage   34.68%   32.94%   -1.74%     
==========================================
  Files         302      269      -33     
  Lines       36923    31289    -5634     
==========================================
- Hits        12806    10308    -2498     
+ Misses      24117    20981    -3136     
Files Changed Coverage Δ
src/agent/onefuzz-agent/src/worker.rs 48.63% <ø> (+1.16%) ⬆️
src/agent/onefuzz/src/memory.rs 61.11% <ø> (+36.11%) ⬆️

... and 55 files with indirect coverage changes

@Porges Porges force-pushed the windows-rs branch 4 times, most recently from ac6c1c6 to 685b91b Compare April 27, 2023 03:27
@Porges
Copy link
Member Author

Porges commented Jul 18, 2023

I've checked coverage output with check-pr, which is the main thing that I would expect to be affected by this change.

@Porges Porges marked this pull request as ready for review July 18, 2023 03:41
@shuffle2
Copy link

shuffle2 commented Jul 18, 2023

Just cloned this repo and tried to build with winapi v0.3.9. The build (of src\agent\coverage) is broken as synchapi and impl-default aren't enabled. So I guess this PR would be nice as it should happen to fix the build too :)

@Porges
Copy link
Member Author

Porges commented Jul 19, 2023

Just cloned this repo and tried to build with winapi v0.3.9. The build (of src\agent\coverage) is broken as synchapi and impl-default aren't enabled. So I guess this PR would be nice as it should happen to fix the build too :)

The issue here is that it depends on what level in the repo you build at. If you build src/agent then all the features of winapi which we need are enabled, since cargo will union them all together. However, some of the subcrates use features without enabling them (for example, I just fixed one over here), so you might get failures if you build an individual crate. This is annoying to detect in the CI build since we'd have to build each crate individually rather than just building the whole workspace...

@Porges Porges enabled auto-merge (squash) August 8, 2023 21:02
@Porges Porges merged commit 58447f7 into main Aug 8, 2023
24 checks passed
@AdamL-Microsoft AdamL-Microsoft mentioned this pull request Aug 10, 2023
@Porges Porges deleted the windows-rs branch September 7, 2023 23:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants