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

Accept web3 1.2.0 as valid version #63

Merged

Conversation

cgewecke
Copy link
Contributor

@cgewecke cgewecke commented Jul 26, 2019

Hi, am seeing a handlful of CI failures today in projects that float their truffle version and use the zeppelin helpers. Their latest release (v5.0.29) upgrades to the new Web3 stable (v1.2.0) and triggers this error:

Error: web3@1.2.0 detected, incompatible with requirement of web3@1.0.0-beta.37
    at setWeb3 (/home/circleci/project/openzeppelin-solidity/node_modules/openzeppelin-test-helpers/src/configure-web3.js:18:11)
    at defaultConfigure (/home/circleci/project/openzeppelin-solidity/node_modules/openzeppelin-test-helpers/configure.js:48:3)

Examples
gnosis-hg greenkeeper CI
solidity-coverage nightly CI

Additional context about semver.satisfies

semver.satisfies('2.0.0-alpha', '>=1.2.0')
> false
semver.satisfies('2.0.0-alpha.1', '>=1.2.0')
> false
semver.satisfies('1.2.1', '>=1.2.0')
> true
semver.satisfies('2.0.0', '>=1.2.0')
> true

Another possibility is 1.0.0-beta.37 || 1.x but there are some (wrongly) published versions below 1.2....

semver.satisfies('1.2.1', '1.0.0-beta.37 || 1.x')
> true
semver.satisfies('1.0.0-beta.36', '1.0.0-beta.37 || 1.x')
> false
semver.satisfies('2.0.0-alpha', '1.0.0-beta.37 || 1.x')}
> false
semver.satisfies('2.0.0-alpha.1', '1.0.0-beta.37 || 1.x')
> false
semver.satisfies('2.0.0', '1.0.0-beta.37 || 1.x')
> false

Fixes #62

@nventuro
Copy link
Contributor

Hey there @cgewecke, thank you very much for tackling this! We've just got a report about this from @ccolorado in #62, but this seems to be a bit more widespread than I anticipated.

I'll thrown an integration test to make sure this stays fixed, and publish the changes as v0.4.1.

semver.satisfies('2.0.0', '>=1.2.0')
> true

I worry this may cause issues later down the road, perhaps it'd be best to use ^ instead of >=?

Also, just out of curiosity, how did you stumble upon these CI failures? Are you running some sort of monitoring tool, or was it simply a coincidence?

@cgewecke
Copy link
Contributor Author

cgewecke commented Jul 26, 2019

was it simply a coincidence

Yes - I'm just watching them on GH and got notifications. Solidity-coverage is my CI job and Gnosis installed a tool I'm writing that I want to make sure works ok.

@cgewecke
Copy link
Contributor Author

Ah! I was looking at the semver docs but didn't see the caret ranges - will fix rn.

@cgewecke cgewecke force-pushed the fix/semver-satisfies-web3-stable branch from b7dba0f to be8f811 Compare July 26, 2019 02:53
@nventuro nventuro requested a review from frangio July 26, 2019 03:34
@nventuro
Copy link
Contributor

nventuro commented Jul 26, 2019

I ended up creating a sort of 'template' test from the old truffle integration test, which is now reused on both the 5.0.1 and 5.0.29 tests.

@frangio could you take a quick look at this so we can release a bugfix?

Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Thanks @cgewecke!

@nventuro I left a few comments about the new integration test setup.


cp -r ../simple-project-truffle-template .

sed -i "s/\"truffle\": \"5.0.1\"/\"truffle\": \"5.0.29\"/g" simple-project-truffle-template/package.json
Copy link
Contributor

@frangio frangio Jul 26, 2019

Choose a reason for hiding this comment

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

-i with no arguments is not portable, it doesn't work in macOS. (However, see next comment.)

Suggested change
sed -i "s/\"truffle\": \"5.0.1\"/\"truffle\": \"5.0.29\"/g" simple-project-truffle-template/package.json
sed -i.bak "s/\"truffle\": \"5.0.1\"/\"truffle\": \"5.0.29\"/g" simple-project-truffle-template/package.json
rm simple-project-truffle-template/package.json.bak


cp -r ../simple-project-truffle-template .

sed -i "s/\"truffle\": \"5.0.1\"/\"truffle\": \"5.0.29\"/g" simple-project-truffle-template/package.json
Copy link
Contributor

Choose a reason for hiding this comment

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

I really think it would be best to simply use npm install.

Suggested change
sed -i "s/\"truffle\": \"5.0.1\"/\"truffle\": \"5.0.29\"/g" simple-project-truffle-template/package.json
cd simple-project-truffle-template
npm install truffle@5.0.29


sed -i "s/\"truffle\": \"5.0.1\"/\"truffle\": \"5.0.29\"/g" simple-project-truffle-template/package.json

./simple-project-truffle-template/run.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

If the above suggestion is accepted.

Suggested change
./simple-project-truffle-template/run.sh
./run.sh

@nventuro
Copy link
Contributor

The npm install approach is much better than my sed hack, yes. @cgewecke would you mind accepting those two last suggestions so we can marge this and release? Thanks!

@cgewecke
Copy link
Contributor Author

@nventuro Yes sounds good! Please feel free to modify this PR however you wish. :)

@nventuro nventuro merged commit 40b1547 into OpenZeppelin:master Jul 26, 2019
@cgewecke cgewecke deleted the fix/semver-satisfies-web3-stable branch July 26, 2019 18:21
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.

truffle-contract@4.0.26 seems to break web3 dependencies.
3 participants