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

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

Open
ForbesLindesay opened this issue Feb 5, 2015 · 13 comments

Comments

@ForbesLindesay
Copy link

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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

@indieisaconcept
Copy link

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 springmeyer changed the title Publishing over existing versions Do not error when publishing, existing binary exists, and it is identical Mar 21, 2015
@springmeyer
Copy link
Contributor

@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 #124

@indieisaconcept
Copy link

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
Copy link

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
Copy link
Contributor

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

@indieisaconcept
Copy link

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
Copy link

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

@indieisaconcept
Copy link

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
Copy link
Contributor

@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
Copy link

@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.

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

No branches or pull requests

3 participants