The new version of Regex (1.9.0) is not compatible with 1.8

[sergey-shandar]

Description

The new version of Regex (1.9.0) is not compatible with 1.8 if we use regex::internal::* See

https://github.com/Trust-Machines/stacks-sbtc/actions/runs/5466661304/jobs/9951913250

And the fix:

https://github.com/Trust-Machines/stacks-sbtc/pull/522

Applicable issues

• fixes #

Additional info (benefits, drawbacks, caveats)

Checklist

• Test coverage for new or modified code paths
• Changelog is updated
• Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
• New clarity functions have corresponding PR in clarity-benchmarking repo
• New integration test(s) added to bitcoin-tests.yml

[codecov]

Codecov Report

Merging #3779 (26ad8bd) into master (02dabb8) will increase coverage by 0.12%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3779      +/-   ##
==========================================
+ Coverage    0.06%    0.18%   +0.12%     
==========================================
  Files         301      302       +1     
  Lines      276555   277126     +571     
==========================================
+ Hits          179      512     +333     
- Misses     276376   276614     +238     

Impacted Files	Coverage Δ
clarity/src/vm/costs/mod.rs	0.00% <ø> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[kantai]

Thanks for finding this @sergey-shandar --

I'd suggest just deleting that line from costs/mod.rs instead:

diff --git a/clarity/src/vm/costs/mod.rs b/clarity/src/vm/costs/mod.rs
index ee9a1a6eb..c70ab9600 100644
--- a/clarity/src/vm/costs/mod.rs
+++ b/clarity/src/vm/costs/mod.rs
@@ -18,7 +18,6 @@ use std::collections::{BTreeMap, HashMap};
 use std::convert::{TryFrom, TryInto};
 use std::{cmp, fmt};
 
-use regex::internal::Exec;
 use rusqlite::types::{FromSql, FromSqlResult, ToSql, ToSqlOutput, ValueRef};
 use serde::{Deserialize, Serialize};

The Exec method isn't used anywhere, so we can just remove that import and then we don't need to version pin regex.

[kantai]

Looks great to me

[jcnelson]

I still need clarification about the versioning strategy in the repo.

Just put the full version. Humans look at the cargo.toml file to determine dependencies, not the cargo.lock. The full version removes ambiguity.

…

On Wed, Jul 12, 2023, 11:50 AM Sergey Shandar ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In clarity/Cargo.toml <#3779 (comment)> : > @@ -23,7 +23,7 @@ rand_chacha = "=0.2.2" serde = "1" serde_derive = "1" serde_stacker = "0.1" -regex = "1" +regex = "~1.8" @jcnelson <https://github.com/jcnelson> I still need clarification about the versioning strategy in the repo. Could you comment with your thoughts/requirements on this https://github.com/orgs/Trust-Machines/discussions/502 ? — Reply to this email directly, view it on GitHub <#3779 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADQJK4KQ3OXQQUOJ4LOFZLXP3BTXANCNFSM6AAAAAA2AT6UYM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[kantai]

Just put the full version. Humans look at the cargo.toml file to determine dependencies, not the cargo.lock. The full version removes ambiguity.

This PR doesn't touch the Cargo.toml at all right now (because there was a spurious import that could just be removed to solve the issue), and I'd suggest that it stay that way.

If you want to talk about changing the way the blockchain does version specifications in the toml file, that's probably better done in an issue for changing that in the contributing guide.

[jcnelson]

This PR doesn't touch the Cargo.toml at all right now

Ah I see that now. Will approve once I'm not AFK (give me an hour).

…

On Wed, Jul 12, 2023, 2:30 PM Aaron Blankstein ***@***.***> wrote: Just put the full version. Humans look at the cargo.toml file to determine dependencies, not the cargo.lock. The full version removes ambiguity. This PR doesn't touch the Cargo.toml at all right now (because there was a spurious import that could just be removed to solve the issue), and I'd suggest that it stay that way. If you want to talk about changing the way the blockchain does version specifications in the toml file, that's probably better done in an issue for changing that in the contributing guide. — Reply to this email directly, view it on GitHub <#3779 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADQJK4XPJ4JV6H5J544EUDXP3UMVANCNFSM6AAAAAA2AT6UYM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[sergey-shandar]

@kantai is it ok that the PR is open against the master branch? Do we sync changes from the master to the develop and next branches?

[kantai]

@kantai is it ok that the PR is open against the master branch?

Yes -- for changes that don't touch any code (e.g., docs only, github workflows, etc.) it's okay to merge to master. This PR, because it just removes an unused import, is okay.

Do we sync changes from the master to the develop and next branches?

Yes, changes to master should always be merged back into develop and next. There isn't an automated process for this in the github repository.