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

Patch ntapi to restore windows build #31961

Merged
merged 6 commits into from
Jun 5, 2023
Merged

Conversation

ryoqun
Copy link
Member

@ryoqun ryoqun commented Jun 4, 2023

Problem

It's been awhile we temporarily stopped Windows build due to incompatibility between one of transitive deps (ntapi) and the solana monorepo's rust toolchain version (v1.69)....:

#31353
#31893

Unfortunately, it's not easy to update to ntapi v0.4.x (which contains a fix for the incompatibility), due to its being middle of our gigantic dep. graph (beneath our somewhat outdated tokio...). On top of ti, the crate maintainer hasn't been responsive with regard to backporting the fix into v0.3.x.

Summary of Changes

Patch ntapi: solana-labs/ntapi@5980bba :

~/work/ntapi$ git diff --no-index /home/ryoqun/.cargo/registry/src/github.com-1ecc6299db9ec823/ntapi-0.3.6/src/ ./src/
diff --git a/home/ryoqun/.cargo/registry/src/github.com-1ecc6299db9ec823/ntapi-0.3.6/src/ntexapi.rs b/./src/ntexapi.rs
index 7d34220..72184bd 100644
--- a/home/ryoqun/.cargo/registry/src/github.com-1ecc6299db9ec823/ntapi-0.3.6/src/ntexapi.rs
+++ b/./src/ntexapi.rs
@@ -1,5 +1,7 @@
 #[allow(deprecated)] //fixme
 use core::mem::uninitialized;
+#[cfg(any(target_arch = "x86_64", target_arch = "aarch64"))]
+use core::ptr::addr_of;
 use core::ptr::read_volatile;
 #[cfg(target_arch = "x86")]
 use core::sync::atomic::spin_loop_hint;
@@ -2782,7 +2784,7 @@ pub unsafe fn NtGetTickCount64() -> ULONGLONG {
     #[allow(deprecated)] //fixme
     let mut tick_count: ULARGE_INTEGER = uninitialized();
     #[cfg(any(target_arch = "x86_64", target_arch = "aarch64"))] {
-        *tick_count.QuadPart_mut() = read_volatile(&(*USER_SHARED_DATA).u.TickCountQuad);
+        *tick_count.QuadPart_mut() = read_volatile(addr_of!((*USER_SHARED_DATA).u.TickCountQuad));
     }
     #[cfg(target_arch = "x86")] {
         loop {
@@ -2806,7 +2808,7 @@ pub unsafe fn NtGetTickCount64() -> ULONGLONG {
 #[inline]
 pub unsafe fn NtGetTickCount() -> ULONG {
     #[cfg(any(target_arch = "x86_64", target_arch = "aarch64"))] {
-        ((read_volatile(&(*USER_SHARED_DATA).u.TickCountQuad)
+        ((read_volatile(addr_of!((*USER_SHARED_DATA).u.TickCountQuad))
             * (*USER_SHARED_DATA).TickCountMultiplier as u64) >> 24) as u32
     }
     #[cfg(target_arch = "x86")] {



~/work/ntapi$ git log -n 2
commit 5980bbab2e0501a8100eb88c12222d664ccb3a0a (HEAD -> master, solana-labs/v0.3.6-for-rust-1.69, ryoqun/v0.3.6-for-rust-1.69)
Author: 包布丁 <htbai1998m@hotmail.com>
Date:   Sat Sep 24 04:26:08 2022 +0800

    Fix temporary references for volatile read (#12)

    * Bump Rust toolchain version for CI to 1.64

    * Fix temporary references for volatile read

commit cbea31ea9d624169f01ae0997e6698baceb51867
Author: MSxDOS <15524350+MSxDOS@users.noreply.github.com>
Date:   Fri Oct 23 17:08:17 2020 +0300

    Release 0.3.6



~/work/ntapi$ cat /home/ryoqun/.cargo/registry/src/github.com-1ecc6299db9ec823/ntapi-0.3.6/.cargo_vcs_info.json
{ 
  "git": {
    "sha1": "cbea31ea9d624169f01ae0997e6698baceb51867"
  }
}

the cherry-picked commit is quite small; so it's low risk. also, carrying the patch won't incur much maintenance burden onto us because the upstream crate is itself kind of being unmaintained...

all in all, i think this is win for restoring window build to detect any build issue on Windows as soon as possible. Also, Windows' solana v1.16.x binaries will be available again (after bp-ing this to the branch).

the build fix is confirmed at my fork:

https://github.com/ryoqun/solana/actions/runs/5172165189/jobs/9316255905#step:4:655 :

    Checking tinyvec_macros v0.1.0
    Checking tinyvec v1.5.0
    Checking zeroize v1.3.0
   Compiling ntapi v0.3.6 (https://github.com/ryoqun/ntapi?rev=5980bbab2e0501a8100eb88c12222d664ccb3a0a#5980bbab)
    Checking parking_lot v0.11.2
    Checking aho-corasick v0.7.18
    Checking regex-syntax v0.6.27

@codecov
Copy link

codecov bot commented Jun 5, 2023

Codecov Report

Merging #31961 (6e951b4) into master (828a590) will increase coverage by 0.0%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #31961   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         760      760           
  Lines      207407   207407           
=======================================
+ Hits       169917   169939   +22     
+ Misses      37490    37468   -22     

Cargo.toml Outdated Show resolved Hide resolved
yihau
yihau previously approved these changes Jun 5, 2023
Copy link
Member

@yihau yihau left a comment

Choose a reason for hiding this comment

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

Thank you! I think this one can be the stopgap! but yeah, we need some proper docs for it.
the only concern: please don't rug 😞

@CriesofCarrots do you have any concerns?

btw, a friendly note: this ntapi patch is based on 0.3.6 (which we're using) + a fixed commit (solana-labs/ntapi@5980bba)

@CriesofCarrots
Copy link
Contributor

do you have any concerns?

No, I'm okay using the patch for now.

@ryoqun ryoqun marked this pull request as ready for review June 5, 2023 05:58
@ryoqun ryoqun requested a review from yihau June 5, 2023 05:58
Copy link
Member

@yihau yihau left a comment

Choose a reason for hiding this comment

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

🪖

@@ -3,7 +3,7 @@ name: release-artifacts-auto
on:
push:
branches:
# - master
- master
Copy link
Member Author

Choose a reason for hiding this comment

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

hehe, i included this change as well in this pr... 🙏

@ryoqun ryoqun merged commit 39770b2 into solana-labs:master Jun 5, 2023
20 checks passed
ryoqun added a commit to ryoqun/solana that referenced this pull request Jun 6, 2023
* Patch ntapi to restore windows build

* Update Cargo.lock...

* Add comment for justification of this patching

MSxDOS/ntapi#11
MSxDOS/ntapi#12

* Revert "ci: stop windows building on master temporarily (solana-labs#31353)"

This reverts commit 2dcdfff.

* Use solana-labs fork

* Ugh..
ryoqun added a commit that referenced this pull request Jun 6, 2023
* Shift crossbeam comment for upcoming 2nd patch... (#31963)

* Patch ntapi to restore windows build (#31961)

* Patch ntapi to restore windows build

* Update Cargo.lock...

* Add comment for justification of this patching

MSxDOS/ntapi#11
MSxDOS/ntapi#12

* Revert "ci: stop windows building on master temporarily (#31353)"

This reverts commit 2dcdfff.

* Use solana-labs fork

* Ugh..

* Patch ntapi more thoroughly (#31970)

* Patch spl-token-cli build as well...

* Patch sbf/Cargo.toml for consistency

* Remove --locked for cli-arg based patch... (#31971)

* Bump patched ntapi from v0.3.6 to v0.3.7 (#31981)

* Revert "ci: stop windows build (#31893)"

This reverts commit 30f9e43.
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.

None yet

3 participants