mapbox / node-pre-gyp

Node.js tool for easy binary deployment of C++ addons
BSD 3-Clause "New" or "Revised" License
1.11k stars 263 forks source link

Do not error when publishing, existing binary exists, and it is identical #136

Open ForbesLindesay opened 9 years ago

ForbesLindesay commented 9 years ago

At the moment, node-pre-gyp package publish errors when there is already an existing version. It would be great if it could download the existing version and only error if the new version is not the same as the existing version.

springmeyer commented 9 years ago

Yes, this is by design in that npm publish also works this way to make it hard to break already released binaries.

If you want to overwrite you can unpublish first. I assume you figured this out but want to have Travis or appveyor jobs only fail if binaries change (to know if an unpublish might be needed during development)?

We could start uploading a shasum with binaries to make it easier to see if a binary has changed. but I like your idea of downloading on the fly since that is simplier.

ForbesLindesay commented 9 years ago

Yes, What's happening is that travis might succeed with mac and linux but appveyor might fail on windows, so I want to make some changes that should only impact the windows build and then republish. But now even if windows succeeds, I'll get build errors from the mac and linux versions.

springmeyer commented 9 years ago

Okay, thanks for the explanation. I think your idea is sound and I'd accept a pull request changing this behavior.

indieisaconcept commented 9 years ago

If you want to overwrite you can unpublish first. I assume you figured this out but want to have Travis or appveyor jobs only fail if binaries change (to know if an unpublish might be needed during development)?

Consider also when two versions of node share the same "node_abi". This is currently the case with later versions of 0.11 & 12, they both have 14. Un-publishing during a travis build could actually break one or both builds depending upon when they run.

springmeyer commented 9 years ago

@indieisaconcept - just edited title to avoid confusion. The idea here is not to make unpublishing any easier: it should stay as a separate command and node-pre-gyp publish should continue to not overwrite binaries. But the improvement that could be made is to have node-pre-gyp publish not error if an existing binary exists and is identical: it would then only error if the binary attempting to be published is different that the remote. In that case you'd need to manually unpublish if you you truly want to replace it (and therefore opt-in to the risk of replacing the binary).

Consider also when two versions of node share the same "node_abi". This is currently the case with later versions of 0.11 & 12, they both have 14.

True except that recent node-pre-gyp uses major.minor.patch as the ABI instead process.versions.modules for even releases as per https://github.com/mapbox/node-pre-gyp/issues/124

indieisaconcept commented 9 years ago

True except that recent node-pre-gyp uses major.minor.patch as the ABI instead process.versions.modules for even releases as per #124

Thanks @springmeyer for clarifying the behaviour. I just realised I'm using an older version. i think I copied it from the example which was out of date. I tested and I now get the behaviour i want.

indieisaconcept commented 9 years ago

You shouldn't have to download the file in-order to determine if the publish can proceed. I've just verified something along the lines of following works with the intention of submitting a PR.

However the ETag for the remote file will not match the locally generated file as the creation date of both will not be the same. I still think it's possible to do a comparison this way though, if I can work around the creation date.

AWS.util.crypto.hash('md5', fs.createReadStream(tarball), 'hex', function (err, checksum) {

  var ETag = meta.ETag && meta.ETag.slice(1, meta.ETag.length - 1);

  if (checksum !== ETag) {
      log.error('publish','Cannot publish over existing version');
      log.error('publish',"Update the 'version' field in package.json and try again");
      log.error('publish','If the previous version was published in error see:');
      log.error('publish','\t node-pre-gyp unpublish');
      return callback(new Error('Failed publishing to ' + remote_package));
  }

  log.info('publish', 'object already exists');
  return callback();

});
springmeyer commented 9 years ago

Great: not having to download the binary in order to figure out whether to error or not would be nice.

indieisaconcept commented 9 years ago

So it would appear that node-gyp is possibly introducing a build-time timestamp or something similar. I've just done some testing and with each rebuild of a module its checksum is different.

I'm presently trying to track-down where in the code it is doing it in the hope it's just a flag I can pass to prevent it.

indieisaconcept commented 9 years ago

Issue raised with TooTallNate/node-gyp/issues/597 to hopefully shed some light on what is going on.

indieisaconcept commented 9 years ago

Further experimentation has lead me to believe it might not be possible to create a deterministic binary for the purpose of generating a comparable shasum. Using -frandom-seed is not supported by Clang, Equally it shouldn't really require end users to modify their bindings to add this flag or similar.

Something other than the binary may have to be compared, uploaded as part of the initial publish and then using the headObject request compare it's Etag against a locally.

springmeyer commented 9 years ago

@indieisaconcept - thanks for the followup. This is surprising, but I trust what you are seeing. Makes me really wonder at what level the binaries are different. Can you isolate the problem with clang++ alone? (you can get the clang++ compile args by doing npm --verbose --build-from-source. Or maybe the problem is at the make level.

indieisaconcept commented 9 years ago

@springmeyer I'm not an expect when it comes to C or compiling C. But I have been able to predictably observe this behavior at least in the module I am testing against. strongloop/fsevents. I'll pull down the example project and do some testing with that.