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.

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

#293 Set MSRV to 1.70.0 #298

Closed
wants to merge 23 commits into from
Closed

#293 Set MSRV to 1.70.0 #298

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
d168e8f
Set up MSRV 1.70.0
diehuxx Aug 20, 2024
bd1dc09
Simplify
diehuxx Aug 20, 2024
c46111f
Support 1.70.0
diehuxx Aug 20, 2024
e8f5667
Lint
diehuxx Aug 20, 2024
774487f
Only check web5 crate for msrv
diehuxx Aug 20, 2024
a4f23df
Only MSRV check with ubuntu
diehuxx Aug 20, 2024
4088c18
Use path option
diehuxx Aug 20, 2024
90ed3c4
just setup in CI
diehuxx Aug 20, 2024
93a731a
Allow rustc 1.80.0 outside of web5 crate
diehuxx Aug 20, 2024
1bc1fd0
install MSRV in CI action
diehuxx Aug 20, 2024
1fb9260
Only check MSRV with stable
diehuxx Aug 20, 2024
2f780b9
Properly install msrv in CI
diehuxx Aug 20, 2024
8fc8b0e
Use 1.70.0 everywhere
diehuxx Aug 20, 2024
bec796e
Use rust-toolchain.toml to set 1.70.0 just in web5 crate
diehuxx Aug 21, 2024
554a974
Scope MSRV check back to web5 crate
diehuxx Aug 21, 2024
b6fc562
Update just test and just build to use both rustc versions
diehuxx Aug 21, 2024
aa422f5
1.74.0 through the repo
diehuxx Aug 21, 2024
22fd4a1
1.75.0
diehuxx Aug 21, 2024
a6aef12
Merge branch 'main' into msrv-1.70.0
diehuxx Aug 21, 2024
af762e2
Remove LazyLock
diehuxx Aug 21, 2024
83e83a9
Remove unused import
diehuxx Aug 22, 2024
120525c
Simplify
diehuxx Aug 22, 2024
762fe21
Lint
diehuxx Aug 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,23 @@ jobs:
- name: Lint
run: just lint

msrv:
strategy:
matrix:
os: [ubuntu-latest, macos-latest] # TODO add back windows-latest https://github.com/TBD54566975/web5-rs/issues/189
KendallWeihe marked this conversation as resolved.
Show resolved Hide resolved
rust: [stable, nightly]
runs-on: ${{ matrix.os }}
steps:
- uses: actions/checkout@v4
with:
submodules: true
- name: Init Hermit
uses: cashapp/activate-hermit@v1
with:
cache: true
- name: MSRV
run: cargo msrv verify --min 1.70.0

test:
strategy:
matrix:
Expand Down
2 changes: 1 addition & 1 deletion Justfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ setup:
git submodule update --init --recursive
fi
if [[ "$(cargo 2>&1)" == *"rustup could not choose a version of cargo to run"* ]]; then
rustup default 1.80.0
rustup default 1.70.0
rustup target add aarch64-apple-darwin
fi

Expand Down
2 changes: 2 additions & 0 deletions crates/web5/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ edition = "2021"
homepage.workspace = true
repository.workspace = true
license-file.workspace = true
rust-version = "1.70.0"

[dependencies]
base64 = { workspace = true }
Expand All @@ -27,6 +28,7 @@ thiserror = { workspace = true }
url = "2.5.0"
uuid = { workspace = true }
zbase32 = "0.1.2"
lazy_static = "1.5.0"

[dev-dependencies]
tokio = { version = "1.38.0", features = ["macros", "test-util"] }
53 changes: 28 additions & 25 deletions crates/web5/src/credentials/verifiable_credential_1_1.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,9 +551,12 @@ mod tests {
const ISSUER_DID_URI: &str = "did:web:tbd.website";
const SUBJECT_DID_URI: &str = "did:dht:qgmmpyjw5hwnqfgzn7wmrm33ady8gb8z9ideib6m9gj4ys6wny8y";

static ISSUER: LazyLock<Issuer> = LazyLock::new(|| Issuer::from(ISSUER_DID_URI));
static CREDENTIAL_SUBJECT: LazyLock<CredentialSubject> =
LazyLock::new(|| CredentialSubject::from(SUBJECT_DID_URI));
fn issuer() -> Issuer {
Issuer::from(ISSUER_DID_URI)
}
fn credential_subject() -> CredentialSubject {
CredentialSubject::from(SUBJECT_DID_URI)
}

mod create {
use super::*;
Expand All @@ -578,7 +581,7 @@ mod tests {
fn test_default_context_added_if_not_supplied() {
TEST_SUITE.include(test_name!());

let vc = VerifiableCredential::create(ISSUER.clone(), CREDENTIAL_SUBJECT.clone(), None)
let vc = VerifiableCredential::create(issuer(), credential_subject(), None)
.unwrap();

assert_eq!(vc.context, vec![BASE_CONTEXT]);
Expand All @@ -594,7 +597,7 @@ mod tests {
});

let vc =
VerifiableCredential::create(ISSUER.clone(), CREDENTIAL_SUBJECT.clone(), options)
VerifiableCredential::create(issuer(), credential_subject(), options)
.unwrap();

assert_eq!(vc.context, vec![BASE_CONTEXT]);
Expand All @@ -611,7 +614,7 @@ mod tests {
});

let vc =
VerifiableCredential::create(ISSUER.clone(), CREDENTIAL_SUBJECT.clone(), options)
VerifiableCredential::create(issuer(), credential_subject(), options)
.unwrap();

assert_eq!(vc.context, vec![BASE_CONTEXT, custom_context]);
Expand All @@ -621,7 +624,7 @@ mod tests {
fn test_default_type_added_if_not_supplied() {
TEST_SUITE.include(test_name!());

let vc = VerifiableCredential::create(ISSUER.clone(), CREDENTIAL_SUBJECT.clone(), None)
let vc = VerifiableCredential::create(issuer(), credential_subject(), None)
.unwrap();

assert_eq!(vc.r#type, vec![BASE_TYPE]);
Expand All @@ -637,7 +640,7 @@ mod tests {
});

let vc =
VerifiableCredential::create(ISSUER.clone(), CREDENTIAL_SUBJECT.clone(), options)
VerifiableCredential::create(issuer(), credential_subject(), options)
.unwrap();

assert_eq!(vc.r#type, vec![BASE_TYPE]);
Expand All @@ -654,7 +657,7 @@ mod tests {
});

let vc =
VerifiableCredential::create(ISSUER.clone(), CREDENTIAL_SUBJECT.clone(), options)
VerifiableCredential::create(issuer(), credential_subject(), options)
.unwrap();

assert_eq!(vc.r#type, vec![BASE_TYPE, custom_type]);
Expand All @@ -664,7 +667,7 @@ mod tests {
fn test_id_generated_if_not_supplied() {
TEST_SUITE.include(test_name!());

let vc = VerifiableCredential::create(ISSUER.clone(), CREDENTIAL_SUBJECT.clone(), None)
let vc = VerifiableCredential::create(issuer(), credential_subject(), None)
.unwrap();

let uuid_regex = Regex::new(r"^urn:uuid:[0-9a-fA-F-]{36}$").unwrap();
Expand All @@ -682,7 +685,7 @@ mod tests {
});

let vc =
VerifiableCredential::create(ISSUER.clone(), CREDENTIAL_SUBJECT.clone(), options)
VerifiableCredential::create(issuer(), credential_subject(), options)
.unwrap();

assert_eq!(vc.id, custom_id);
Expand All @@ -694,7 +697,7 @@ mod tests {

let empty_issuer = Issuer::from("");
let result =
VerifiableCredential::create(empty_issuer, CREDENTIAL_SUBJECT.clone(), None);
VerifiableCredential::create(empty_issuer, credential_subject(), None);

match result {
Err(Web5Error::Parameter(err_msg)) => {
Expand All @@ -708,10 +711,10 @@ mod tests {
fn test_issuer_string_must_be_set() {
TEST_SUITE.include(test_name!());

let vc = VerifiableCredential::create(ISSUER.clone(), CREDENTIAL_SUBJECT.clone(), None)
let vc = VerifiableCredential::create(issuer(), credential_subject(), None)
.unwrap();

assert_eq!(vc.issuer, ISSUER.clone());
assert_eq!(vc.issuer, issuer());
}

#[test]
Expand All @@ -724,7 +727,7 @@ mod tests {
additional_properties: None,
});

let result = VerifiableCredential::create(issuer, CREDENTIAL_SUBJECT.clone(), None);
let result = VerifiableCredential::create(issuer, credential_subject(), None);

match result {
Err(Web5Error::Parameter(err_msg)) => {
Expand All @@ -744,7 +747,7 @@ mod tests {
additional_properties: None,
});

let result = VerifiableCredential::create(issuer, CREDENTIAL_SUBJECT.clone(), None);
let result = VerifiableCredential::create(issuer, credential_subject(), None);

match result {
Err(Web5Error::Parameter(err_msg)) => {
Expand All @@ -764,7 +767,7 @@ mod tests {
additional_properties: None,
});

let vc = VerifiableCredential::create(issuer.clone(), CREDENTIAL_SUBJECT.clone(), None)
let vc = VerifiableCredential::create(issuer.clone(), credential_subject(), None)
.unwrap();

assert_eq!(vc.issuer, issuer);
Expand All @@ -787,7 +790,7 @@ mod tests {
additional_properties: Some(additional_properties.clone()),
});

let vc = VerifiableCredential::create(issuer.clone(), CREDENTIAL_SUBJECT.clone(), None)
let vc = VerifiableCredential::create(issuer.clone(), credential_subject(), None)
.unwrap();

match vc.issuer {
Expand All @@ -804,7 +807,7 @@ mod tests {

let credential_subject = CredentialSubject::from("");

let result = VerifiableCredential::create(ISSUER.clone(), credential_subject, None);
let result = VerifiableCredential::create(issuer(), credential_subject, None);

match result {
Err(Web5Error::Parameter(err_msg)) => {
Expand All @@ -818,10 +821,10 @@ mod tests {
fn test_credential_subject_must_be_set() {
TEST_SUITE.include(test_name!());

let vc = VerifiableCredential::create(ISSUER.clone(), CREDENTIAL_SUBJECT.clone(), None)
let vc = VerifiableCredential::create(issuer(), credential_subject(), None)
.unwrap();

assert_eq!(vc.credential_subject, CREDENTIAL_SUBJECT.clone());
assert_eq!(vc.credential_subject, credential_subject());
}

#[test]
Expand All @@ -840,7 +843,7 @@ mod tests {
additional_properties: Some(additional_properties.clone()),
};

let vc = VerifiableCredential::create(ISSUER.clone(), credential_subject.clone(), None)
let vc = VerifiableCredential::create(issuer(), credential_subject.clone(), None)
.unwrap();

assert_eq!(
Expand All @@ -861,7 +864,7 @@ mod tests {
});

let vc =
VerifiableCredential::create(ISSUER.clone(), CREDENTIAL_SUBJECT.clone(), options)
VerifiableCredential::create(issuer(), credential_subject(), options)
.unwrap();

assert_eq!(vc.issuance_date, issuance_date);
Expand All @@ -871,7 +874,7 @@ mod tests {
fn test_issuance_date_must_be_now_if_not_supplied() {
TEST_SUITE.include(test_name!());

let vc = VerifiableCredential::create(ISSUER.clone(), CREDENTIAL_SUBJECT.clone(), None)
let vc = VerifiableCredential::create(issuer(), credential_subject(), None)
.unwrap();

let now = SystemTime::now();
Expand All @@ -891,7 +894,7 @@ mod tests {
});

let vc =
VerifiableCredential::create(ISSUER.clone(), CREDENTIAL_SUBJECT.clone(), options)
VerifiableCredential::create(issuer(), credential_subject(), options)
.unwrap();

assert_eq!(vc.expiration_date, Some(expiration_date));
Expand Down
58 changes: 28 additions & 30 deletions crates/web5/src/dids/did.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
use crate::errors::{Result, Web5Error};
use lazy_static::lazy_static;
use regex::Regex;
use std::fmt;
use std::{collections::HashMap, sync::LazyLock};
use std::collections::HashMap;

#[derive(Debug, Clone, Default, PartialEq)]
pub struct Did {
Expand All @@ -28,36 +29,31 @@ static PATH_INDEX: usize = 6;
static QUERY_INDEX: usize = 7;
static FRAGMENT_INDEX: usize = 8;

static DID_URL_PATTERN: LazyLock<Regex> = LazyLock::new(|| {
// relevant ABNF rules: https://www.w3.org/TR/did-core/#did-syntax
let pct_encoded_pattern: &str = r"(?:%[0-9a-fA-F]{2})";
let method_pattern: &str = r"([a-z0-9]+)";
let param_char_pattern: &str = r"[a-zA-Z0-9_.:%-]";
let path_pattern: &str = r"(/[^#?]*)?";
let query_pattern: &str = r"(\?[^\#]*)?";
let fragment_pattern: &str = r"(\#.*)?";
let id_char_pattern = format!(r"(?:[a-zA-Z0-9._-]|{})", pct_encoded_pattern);
let method_id_pattern = format!(r"((?:{}*:)*({}+))", id_char_pattern, id_char_pattern);
let param_pattern = format!(r";{}+={}*", param_char_pattern, param_char_pattern);
let params_pattern = format!(r"(({})*)", param_pattern);

Regex::new(&format!(
r"^did:{}:{}{}{}{}{}$",
method_pattern,
method_id_pattern,
params_pattern,
path_pattern,
query_pattern,
fragment_pattern
))
.map_err(|e| {
Web5Error::Parameter(format!(
"DID_URL_PATTERN regex instantiation failure: {}",
e
lazy_static! {
static ref DID_URL_PATTERN: Regex = {
let pct_encoded_pattern: &str = r"(?:%[0-9a-fA-F]{2})";
let method_pattern: &str = r"([a-z0-9]+)";
let param_char_pattern: &str = r"[a-zA-Z0-9_.:%-]";
let path_pattern: &str = r"(/[^#?]*)?";
let query_pattern: &str = r"(\?[^\#]*)?";
let fragment_pattern: &str = r"(\#.*)?";
let id_char_pattern = format!(r"(?:[a-zA-Z0-9._-]|{})", pct_encoded_pattern);
let method_id_pattern = format!(r"((?:{}*:)*({}+))", id_char_pattern, id_char_pattern);
let param_pattern = format!(r";{}+={}*", param_char_pattern, param_char_pattern);
let params_pattern = format!(r"(({})*)", param_pattern);

Regex::new(&format!(
r"^did:{}:{}{}{}{}{}$",
method_pattern,
method_id_pattern,
params_pattern,
path_pattern,
query_pattern,
fragment_pattern
))
})
.unwrap() // immediately panic on startup if regex is faulty, this will assist in shift-left development
});
.unwrap()
};
}

impl Did {
pub fn parse(uri: &str) -> Result<Did> {
Expand Down Expand Up @@ -115,6 +111,8 @@ mod tests {
use super::*;

mod new {
use std::sync::LazyLock;

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
use std::sync::LazyLock;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests don't affect MSRV

Copy link
Contributor

Choose a reason for hiding this comment

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

oh interesting, this is still used? was it in preview for 1.70.0 or something and so it's okay inside of tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

we commented at the same time

so but even though our rustup is set to 1.70.0 in our Justfile, this still works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Updated Justfile back to 1.80.0 and we'll just have the MSRV check run separately. Makes for an easier dev environment for the places where we don't need to worry about MSRV

Copy link
Contributor

Choose a reason for hiding this comment

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

I kind of feel like we should just full commit to whatever our MSRV is, even in our local dev env, such that we don't develop only to find out that we have an incompatible feature until the CI pipeline is ran. Or would the rust-version property in the Cargo.toml already enforce that? Like if I have 1.80.0 in my local env, then I set the rust-version to 1.70.0 in my Cargo.toml and then I use a LazyLock in non-test code, and I run cargo build will it fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like if I have 1.80.0 in my local env, then I set the rust-version to 1.70.0 in my Cargo.toml and then I use a LazyLock in non-test code, and I run cargo build will it fail?

It will not. It's only after the MSRV check that it would get flagged.

we should just full commit to whatever our MSRV is

Word, let me try it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We install packages during CI that require above 1.70.0. If we want to run everything on 1.70.0, not just the web5 crate where it is useful to lower MSRV, then we'll need to refactor those aspects of the CI and remove LazyLock from our tests. @KendallWeihe do you still want to use 1.70.0 for the entire codebase?

Copy link
Contributor Author

@diehuxx diehuxx Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Another option is to use crates/web5/rust-toolchain.toml to build and test the web5 crate with rustc 1.70.0, while using 1.80.0 in the rest of the codebase. rustc 1.70.0 will only be used when the current directory is in the crates/web5/ directory.

Edit: in this case, we'll need to remove LazyLock from our tests

Copy link
Contributor

Choose a reason for hiding this comment

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

I think my proposal is we go with 1.74.0 for right now #303

And then let's open a ticket on the backlog to see if we can push it further lower at a later date

use super::*;
use crate::{test_helpers::UnitTestSuite, test_name};

Expand Down
Loading