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

Enable CI #1

Open
wants to merge 50 commits into
base: main
Choose a base branch
from
Open

Enable CI #1

wants to merge 50 commits into from

Conversation

chriskilding
Copy link
Collaborator

Add GitHub Actions, and the necessary extras for that automated build.

@chriskilding
Copy link
Collaborator Author

Keeping this in Draft while I work on it - don't merge yet

@chriskilding
Copy link
Collaborator Author

Related discussion in nodejs/node#39657

@chriskilding
Copy link
Collaborator Author

@bnoordhuis the first error we encounter in the Action (which is basically the standard Github Actions template for a Node project) is that it expects a package-lock.json (or equivalent), but there isn't one.

This is strange to me because application projects typically have lockfiles, but libraries typically do not. (At least, prominent libraries I've seen like ExpressJS don't have them.)

Any thoughts about whether to add one?

Copy link
Owner

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great! Couple of questions:

  1. Doesn't this need to install rust/cargo with e.g. https://github.com/marketplace/actions/rust-toolchain or a uses: dtolnay/rust-toolchain@stable?

  2. Can npm publish be made to work now that it requires 2FA? I was thinking I'd download build artifacts and publish them to npm manually but automation > manual labor, of course.

My plan of attack was to bundle .node files for x64 Linux, x64 macOS and ia32 and x64 Windows and maybe arm64 macOS (if people ask for it and I can get it to cross-compile.)

@bnoordhuis
Copy link
Owner

Apropos package-lock.json, libraries indeed shouldn't have one. Where are you seeing that error?

@chriskilding
Copy link
Collaborator Author

I was seeing it when the test workflow was calling npm ci (which apparently always requires a lockfile). I have circumvented that for now by using npm install (where a lockfile is optional), which allows us to advance to the important bit of the build - getting the test working.

I'm not sure what the recommendation is for libraries nowadays - whether to use ci or install?

@chriskilding
Copy link
Collaborator Author

And yes, in due course the Action will need to set up Rust and friends

@chriskilding
Copy link
Collaborator Author

Ignore previous comment - looks like a version of Rust is preinstalled on the GH Actions runners, so the build phase completed successfully. We'd only need to override it with custom Rust setup if your Rust code (a) needs a particular Rust version or (b) uses extra Rust components (e.g. rustfmt, clippy).

@chriskilding
Copy link
Collaborator Author

Here comes the fun part...

> node test.js
node:internal/modules/cjs/loader:936
  throw err;
  ^
Error: Cannot find module './binding.node'
Require stack:
- /home/runner/work/node-native-certs/node-native-certs/index.js
- /home/runner/work/node-native-certs/node-native-certs/test.js
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:933:15)
    at Function.Module._load (node:internal/modules/cjs/loader:778:27)
    at Module.require (node:internal/modules/cjs/loader:1005:19)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Object.<anonymous> (/home/runner/work/node-native-certs/node-native-certs/index.js:3:27)
    at Module._compile (node:internal/modules/cjs/loader:1105:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1159:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Module.require (node:internal/modules/cjs/loader:1005:19) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/home/runner/work/node-native-certs/node-native-certs/index.js',
    '/home/runner/work/node-native-certs/node-native-certs/test.js'
  ]
}

@chriskilding
Copy link
Collaborator Author

I'm looking at options for integrating Rust with Node, which would make the build process a bit easier.

One option that looks promising is Neon Bindings (https://neon-bindings.com)

@bnoordhuis
Copy link
Owner

Error: Cannot find module './binding.node'

Oops, mea culpa; that's on me. It's not copying target/release/libnode_native_certs.{dylib,so,dll} to binding.node. I'm afraid I left it in a broken state while trying to figure out multi-platform support.

Apropos Neon, I'm aware of it (first watcher on the repo is yours truly :-)) but I prefer to use Node's official Rust bindings.

@chriskilding
Copy link
Collaborator Author

Is the official binding framework NAPI-RS (https://napi.rs)?

@bnoordhuis
Copy link
Owner

Yes, that's the one.

@chriskilding
Copy link
Collaborator Author

When I ran napi new to generate a hello-world project I noticed it creates some boilerplate, and package structures, that aren't present here e.g. the npm/<cpu-architecture>/... modules. Are they needed to make it work?

@bnoordhuis
Copy link
Owner

Quite possibly. I admit I hacked the current structure together from one of my other projects, and that project only supports one platform.

@chriskilding
Copy link
Collaborator Author

After a bit more work I've managed to get a Win/Mac/Linux x64 matrix build going.

The Rust code compiles and we get to the test stage. The test executes, then fails when https tries to use the provided certs:

Error {
    code: 'UNABLE_TO_GET_ISSUER_CERT_LOCALLY',
    message: 'unable to get local issuer certificate',
}

Having tested locally, the same test will pass when the ca argument is removed i.e. Node uses its own certs.

This seems like things are going in the right direction...

@chriskilding
Copy link
Collaborator Author

I realised the original test (in the main branch) always passed when it should not have done, because https.get is asynchronous and handles the response in a callback, whereas the original test never provided a callback.

@bnoordhuis
Copy link
Owner

Sorry for the delay, I had to read up on GH Packages... it does indeed support proper scopes but is my understanding correct that you can't use it to publish to the npm registry? It looks like it's an alternative npm registry.

@chriskilding
Copy link
Collaborator Author

Yeah it would be an alternative npm registry, which is the main downside of it.

Do you know if it's possible to specify the registry URL per-package? If it is, it might be practical for users to take just node-native-certs from GH Packages. But if it's not possible, then they'd have to set the default registry URL for everything to come from GH Packages, which is not really practical.

@bnoordhuis
Copy link
Owner

You mean in a downstream user's package.json? You can, but people aren't going to find it if it's not on npmjs.com. I'll just publish manually.

@chriskilding
Copy link
Collaborator Author

Yeah looks like its going to be a manual job...

I'm trying to think what this would look like:

  1. We tag a commit we want to release in main, and associate a GitHub Release with it (so that we can write release notes for it).
  2. GH Actions runs a build for that.
  3. You download the compiled binary blobs from that GH Actions build to your laptop, and copy them into the right places (effectively doing the npm artifacts step locally).
  4. You do the npm publish.

@bnoordhuis
Copy link
Owner

Pretty much what I was thinking. :-)

Step 1 can be skipped if it complicates things. I only need Actions for the build artifacts, everything else (tagging, publishing, etc.) I can do locally.

@chriskilding
Copy link
Collaborator Author

I think that if we're releasing manually, it is particularly important to offer some guarantee to users that what they're receiving on NPM matches up with a particular commit.

Also minting a tag and writing GH Release notes is very easy and quick to do.

@bnoordhuis
Copy link
Owner

🤷 no strong feelings. As long as it isn't onerous to use or maintain.

@chriskilding
Copy link
Collaborator Author

Hi again, circling back to this.

In the last week I've been looking at how native certs integration would work across multiple languages, because whether you're doing enterprise TLS inspection, local HTTPS development with self-signed certificates, or ad hoc security work, the presence of the custom certificate causes the same problems in any language stack - not just JS.

To this end I've been working on a Python version of node-native-certs, because my company is also a big Python user. I figure there's value in getting a python-native-certs package out there too.

I've also seen that Ruby has support for building native gems using Rust - and possibly that Bundler or Gem will integrate this as a first-class capability as well, so extra tooling won't be needed to do it. I don't use Ruby much these days, but there are Rails shops out there who do (and the odd CLI tool is written in Ruby), so a ruby-native-certs package could be a good idea too.

To give these things a good place to live, I propose creating a GitHub organization for them, with you and I as members initially. The goal of the organisation would be to see native TLS cert handling advanced to the point where it 'just works' in the major languages. If you're amenable to that, I'll get it set up and then we can transfer this repo into it.

@bnoordhuis
Copy link
Owner

I appreciate the offer but I have only limited time and still suffer PTSD from the last time I tried to publish a python C package. ^_^

@chriskilding chriskilding marked this pull request as ready for review July 7, 2022 10:30
@chriskilding
Copy link
Collaborator Author

In which case I'll go ahead and merge this to main, and then we can try doing a release of it

@bnoordhuis
Copy link
Owner

There were a bunch of unresolved comments last time I looked. Have those been addressed?

@chriskilding
Copy link
Collaborator Author

All of them except one were addressed, which was about the test framework. I would much prefer to keep that given that the cert_format function deserves unit testing. Also if there are any bugs found in future, it will help us to fix them with TDD.

Also Node 18 now has a built-in test framework, so when 18 eventually becomes the standard we can switch to that.

@chriskilding
Copy link
Collaborator Author

Bump - are we good to merge?

@bnoordhuis
Copy link
Owner

Sorry for the delay, the last two weeks have been the perfect storm of COVID outbreaks and... well, mostly COVID.

I have a moderately strong aversion to all third-party testing frameworks. I'm all for testing, but then please use the built-in assert module. :-)

Can you drop the dependencies on the base64 and regex crates? I really don't want to pull those in for something JS can already do.

@chriskilding
Copy link
Collaborator Author

Regarding test frameworks - these not only provide assertions but also a running and standardised reporting mechanism, and a way to structure your test suite when you have more than one test. The assert module only provides assertions, so without a test framework I must solve these other things from scratch. Do you have a way that you do those things in your other code?

@bnoordhuis
Copy link
Owner

I just run tests consecutively. Here's an example: https://github.com/bnoordhuis/node-bursar/blob/master/test.js

My philosophy to testing is that tests should be silent when successful (no reporting needed) and fail immediately and loudly on regressions (hence assert.)

@chriskilding
Copy link
Collaborator Author

Am I right in thinking that you’re using closures to wrap each of the non-one-liner test cases, and hold them together with their local state?

Ie

{
   // ……
  assert.something(…)
}

If so, when one of the closures (tests) fails, are the other tests able to run and you see the reported test failures in the log?

@bnoordhuis
Copy link
Owner

No, it's just straight-line code. The braces are for lexical scoping, nothing more.

are the other tests able to run

That's one of the reasons I hate a lot of testing frameworks: they keep running after a failing test clobbers the process's state, making it that much harder to track down the real cause.

@chriskilding
Copy link
Collaborator Author

The test framework is now removed, replaced with tests in the style of node-bursar

@chriskilding
Copy link
Collaborator Author

...and with the last few commits I've found a way to move the cert formatting code into JS, in a way that does not interfere with the autogenerated JS-from-Rust code.

This has enabled the removal of the base64 and regex crate dependencies - though because rustls-native-certs also depends on them, we don't really avoid their presence by doing this.

@chriskilding
Copy link
Collaborator Author

I think that's everything addressed. Are we good to merge?

@chriskilding
Copy link
Collaborator Author

Bump

@bnoordhuis
Copy link
Owner

Sorry, I haven't forgotten about this but I'm severely lacking in free time at the moment.

@FredeJ
Copy link

FredeJ commented Oct 12, 2022

What's the current status on this? It sounds like you've basically cracked it and just need to push? :)

I'd love to try this out. Something like this could likely solve a whole host of issues on enterprise machines.

@bnoordhuis
Copy link
Owner

npm requires 2FA now (or maybe only if you're a popular author) and it was enough of a pain in the neck to set up that I didn't follow through. I won't hold it against anyone if they fork it and publish under their own account.

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.

3 participants