8000 CI: Fx pinning by tcharding · Pull Request #2036 · rust-bitcoin/rust-bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

CI: Fx pinning #2036

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 31, 2023
Merged

CI: Fx pinning #2036

merged 1 commit into from
Aug 31, 2023

Conversation

tcharding
Copy link
Member
@tcharding tcharding commented Aug 28, 2023 •
edited
Loading

More crates broke our MSRV by using edition 2021 without doing a major release, pin them in the CI script.

Other open PRs need this to get past CI.

@tcharding tcharding mentioned this pull request Aug 28, 2023
Merged
@tcharding
CI: Fix pinning
adcc01c 
More crates broke our MSRV by using edition 2021 without doing a major
release, pin them in the CI script.

diff --git a/contrib/test.sh b/contrib/test.sh
index 74f8ffb..79932ad 100755
--- a/contrib/test.sh
+++ b/contrib/test.sh
@@ -13,6 +13,12 @@ if cargo --version | grep ${MSRV}; then
     cargo update -p quote --precise 1.0.30
     cargo update -p proc-macro2 --precise 1.0.63
     cargo update -p serde_test --precise 1.0.175
+    # Have to pin this so we can pin `schemars_derive`
+    cargo update -p schemars --precise 0.8.12
+    # schemars_derive 0.8.13 uses edition 2021
+    cargo update -p schemars_derive --precise 0.8.12
+    # memcrh 2.6.0 uses edition 2021
+    cargo update -p memchr --precise 2.5.0

     cargo update -p bitcoin:0.30.1 --precise 0.30.0
@tcharding tcharding force-pushed the 08-29-pin-schemars-derive branch from d2b0644 to adcc01c Compare August 29, 2023 02:34
@tcharding tcharding changed the title CI: Pin schemars_derive dependency for MSRV builds CI: Fx pinning Aug 29, 2023
This was referenced Aug 29, 2023
Merged
Merged
Merged
@tcharding tcharding added the P-high High priority label Aug 29, 2023
@yancyribbens
Copy link
Contributor

I'm not able to reproduce the CI failure locally. What am I missing?

checkout 08-29-improve-witness-display:

git checkout remotes/origin/08-29-improve-witness-display

pin deps from README

cargo update -p serde_json --precise 1.0.99
cargo update -p serde --precise 1.0.156
cargo update -p quote --precise 1.0.30
cargo update -p proc-macro2 --precise 1.0.63
cargo update -p serde_test --precise 1.0.175

run cargo check

cargo check --all-features --all-targets

results

  Downloaded rand_chacha v0.3.1                                                                                                                                                         [6/860]
  Downloaded ppv-lite86 v0.2.17
  Downloaded rand_core v0.6.4
  Downloaded schemars v0.8.12
  Downloaded schemars_derive v0.8.12
  Downloaded base64 v0.13.1
  Downloaded rand v0.8.5
  Downloaded memchr v2.5.0
  Downloaded core2 v0.3.3
  Downloaded serde_test v1.0.175
  Downloaded bitcoinconsensus v0.20.2-0.5.0
  Downloaded 16 crates (1.5 MB) in 3.88s
   Compiling memchr v2.5.0
    Checking ryu v1.0.15
   Compiling schemars v0.8.12
    Checking itoa v1.0.9
    Checking cfg-if v1.0.0
    Checking dyn-clone v1.0.12
    Checking ppv-lite86 v0.2.17
   Compiling bitcoin v0.30.0 (/usr/home/yancy/Git/tcharding/rust-bitcoin/bitcoin)
    Checking base64 v0.13.1
    Checking bech32 v0.9.1
    Checking byteorder v1.4.3
    Checking half v1.8.2
   Compiling serde_test v1.0.175
    Checking libc v0.2.147
    Checking hex_lit v0.1.1
    Checking semver v1.0.18
   Compiling secp256k1-sys v0.8.1
   Compiling bitcoinconsensus v0.20.2-0.5.0
   Compiling serde_derive_internals v0.26.0
    Checking serde v1.0.156
    Checking rustc_version v0.4.0
    Checking core2 v0.3.3
    Checking honggfuzz v0.5.55
    Checking getrandom v0.2.10
    Checking hex-conservative v0.1.1
    Checking rand_core v0.6.4
    Checking rand_chacha v0.3.1
    Checking rand v0.8.5
   Compiling schemars_derive v0.8.12
    Checking serde_json v1.0.99
    Checking bitcoin-internals v0.2.0 (/usr/home/yancy/Git/tcharding/rust-bitcoin/internals)
    Checking serde_cbor v0.9.0
    Checking bincode v1.3.3
    Checking bitcoin_hashes v0.12.0 (/usr/home/yancy/Git/tcharding/rust-bitcoin/hashes)
    Checking secp256k1 v0.27.0
    Checking bitcoin-fuzz v0.0.1 (/usr/home/yancy/Git/tcharding/rust-bitcoin/fuzz)
    Finished dev [unoptimized + debuginfo] target(s) in 38.71s

@tcharding
Copy link
Member Author
tcharding commented Aug 30, 2023 •
edited
Loading

Sorry bro, I already spend too much time pinning crates to debug your local repro. It can be seen by the all-green CI that this works and the red 1.48 job in most every other open PR that this is currently broken.

Can we get a second ACK please crew so this can go in. cc @TheBlueMatt because you mentioned your CI was broken here: #2034 (comment)

junderw
junderw approved these changes Aug 30, 2023
View reviewed changes
Copy link
Contributor
@junderw junderw left a comment

Choose a reason for hiding this comment

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

Verified it crashes pre-patch and does not crash post-crash on toolchain 1.48

tACK

@junderw
Copy link
Contributor
junderw commented Aug 30, 2023

@yancyribbens You need to delete Cargo.lock and ./target/ folder after you switch your local toolchain to 1.48

Then try it again from the beginning.

@yancyribbens
Copy link
Contributor

@junderw oops thanks. I figured it was something obvious I forgot about.

sanket1729
sanket1729 approved these changes Aug 30, 2023
View reviewed changes
Copy link
Member
@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

ACK adcc01c

@sanket1729
Copy link
Member

I feel we are short on reviewers. I will merge this tomorrow if we don't get ACKs

@sanket1729
Copy link
Member
sanket1729 commented Aug 30, 2023 •
edited
Loading

I feel we should just merge MSRV CI fixes without waiting for 2 ACKs rule. It stalls development and review for all other PRs.

@tcharding
Copy link
Member Author

I agree, the 2 ack policy is not useful for CI changes in general but especially when CI is broken for other PRs.

@junderw
Copy link
Contributor
junderw commented Aug 30, 2023

I agree. 1 ACK should be enough for CI / testing fixes.

@sanket1729 sanket1729 merged commit c06c9be into rust-bitcoin:master Aug 31, 2023
@apoelstra
Copy link
Member

I wouldn't mind making this explicit in our policy -- I made the same call in some recent PR (which I can't find now). I think if something only touches CI, or only makes grammatical changes to docs (I'm open to other suggestions but let's make them explicit non-judgement-call ones) then it's fine to merge with 1 ACK and no delay.

@tcharding
Copy link
Member Author

#2046

@tcharding tcharding deleted the 08-29-pin-schemars-derive branch September 20, 2023 04:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sanket1729 sanket1729 sanket1729 approved these changes

@junderw junderw junderw approved these changes

Assignees
No one assigned
Labels
P-high High priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tcharding @yancyribbens @junderw @sanket1729 @apoelstra
0