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

jsthemis: Downgrade mocha to ^7 for Centos7 #1003

Merged
merged 4 commits into from
Jun 9, 2023

Conversation

G1gg1L3s
Copy link
Collaborator

@G1gg1L3s G1gg1L3s commented Jun 6, 2023

This version and all its transitive dependencies support node v8 which is the only version we can run on centos 7. It seems like other OS work fine with it, at least on buildbot.

Another option is to fix our scripts on buildbot just for centos 7, so they patch the versions before testing. This may save us from future problems or dependency issues. However, downgrading dev dependency is not that scary, so maybe this approach is okay.

Checklist

  • Change is covered by automated tests
  • Benchmark results are attached (if applicable)
  • The coding guidelines are followed
  • Public API has proper documentation
  • Example projects and code samples are up-to-date (in case of API changes)
  • Changelog is updated (in case of notable or breaking changes)

This version and all its transitive dependencies support node v8
which is the only version we can run on centos 7. It seems like other
OS work fine, at least on buildbot.

Another option is to fix our scripts on buildbot just for centos 7 so
they patch the versions before testing. This may save us from future
problems. However, downgrading dev dependency is not that scary, so
maybe this approach is okay.
@vixentael
Copy link
Contributor

I would NOT downgrade mocha all builds on all platforms if we want to fix only centos 7. Maybe we can create a workaround for centos7 only?

@G1gg1L3s
Copy link
Collaborator Author

G1gg1L3s commented Jun 6, 2023

Sure! We can adjust buildbot scripts: add a step with sed that will replace the version before testing. I don't know other ways of doing this, like adjusting package.json or something.

@radetsky
Copy link
Contributor

radetsky commented Jun 6, 2023

You may try using the scripts section. For example:

,"scripts": {
  "install": "node install_dependencies.js"
}

And use the power of JS to install OS-specific dependencies. Also, it may be a shell script. And call the script before running the tests.

@G1gg1L3s
Copy link
Collaborator Author

G1gg1L3s commented Jun 6, 2023

Hmm, that is an option, thank you! However, it will collide with the node-gyp which we use to build the addon, so in this case we would have to explicitly call the node-gyp. So probably for these reasons npm doesn't recommend redefining install and preinstall. Also, this way is slightly harder to implement. In the end, this is just for tests, the addon itself works fine.

However, if you believe this this the right way, I can do it.

@radetsky
Copy link
Contributor

radetsky commented Jun 6, 2023

You may try using not only "install" or "preinstall" script names. Try "centos7_specific_tests_deps", for example.

Add a separate script for that. It will be caled during make test.
@G1gg1L3s
Copy link
Collaborator Author

G1gg1L3s commented Jun 6, 2023

@radetsky, could you take a quick look please. Does the last commit look like you described?

@radetsky
Copy link
Contributor

radetsky commented Jun 6, 2023

Already did. Thank you. LGTM. Did you test it on centos7 and other OSes?
Anyway, I can not accept it, I'm not the owner.

@G1gg1L3s
Copy link
Collaborator Author

G1gg1L3s commented Jun 6, 2023

Looks like buildbot tests are green, so thank you, @radetsky!

@vixentael vixentael changed the title jsthemis: Downgrade mocha to ^7 jsthemis: Downgrade mocha to ^7 for Centos7 Jun 6, 2023
@vixentael
Copy link
Contributor

Nice one!

@Lagovas WDYT?

@vixentael vixentael added tests Themis test suite W-JsThemis 🍭 Wrapper: JsThemis for Node.js, JavaScript API, npm packages labels Jun 6, 2023
@Lagovas Lagovas merged commit 67e054c into cossacklabs:master Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Themis test suite W-JsThemis 🍭 Wrapper: JsThemis for Node.js, JavaScript API, npm packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants