cossacklabs / themis

Easy to use cryptographic framework for data protection: secure messaging with forward secrecy and secure data storage. Has unified APIs across 14 platforms.
https://www.cossacklabs.com/themis
Apache License 2.0
1.85k stars 143 forks source link

jsthemis: Downgrade mocha to ^7 for Centos7 #1003

Closed G1gg1L3s closed 1 year ago

G1gg1L3s commented 1 year ago

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

vixentael commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

G1gg1L3s commented 1 year ago

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

radetsky commented 1 year ago

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 commented 1 year ago

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

vixentael commented 1 year ago

Nice one!

@Lagovas WDYT?