From 5e72f55eb2dcd1a120e85970fa50ffb1fd72dd00 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Wed, 7 May 2014 14:12:25 -0700 Subject: [PATCH] compare built tarball hash instead of buildid buildids just change too damn much. oh no, you changed a comment in random which is transitively used by templating which processes .html, I guess you need to bump all the versions of all packages with .html? that sucks. how about only changing it if the EFFECT of the build is changed? this does have some subtleties around platform-specific versions but we think we have a good compromise --- tools/catalog.js | 14 +++++++++ tools/commands.js | 63 +++++++++++++++++++++++++---------------- tools/package-client.js | 21 +++++--------- tools/unipackage.js | 5 ++++ 4 files changed, 65 insertions(+), 38 deletions(-) diff --git a/tools/catalog.js b/tools/catalog.js index 30bf60950e..37bb720944 100644 --- a/tools/catalog.js +++ b/tools/catalog.js @@ -841,6 +841,20 @@ _.extend(Catalog.prototype, { return buildsToUse; // We couldn't satisfy it! return null; + }, + + // Unlike the previous, this looks for a build which *precisely* matches the + // given architectures string (joined with +). + getBuildWithArchesString: function (name, version, archesString) { + var self = this; + self._requireInitialized(); + + var versionInfo = self.getVersion(name, version); + if (! versionInfo) + return null; + + return _.where(self.builds, { versionId: versionInfo._id, + architecture: archesString } ) || null; } }); diff --git a/tools/commands.js b/tools/commands.js index 81263cc7e3..49b5f161b2 100644 --- a/tools/commands.js +++ b/tools/commands.js @@ -1757,7 +1757,7 @@ main.registerCommand({ packageClient.handlePackageServerConnectionError(err); return 1; } - packageClient.createAndPublishBuiltPackage(conn, unipackage, packageDir); + packageClient.createAndPublishBuiltPackage(conn, unipackage); return 0; }); @@ -1893,7 +1893,8 @@ main.registerCommand({ // it doesn't we should fail. Hopefully, of course, we have // tested our stuff before deciding to publish it to the package // server, but we need to be careful. - var compileResult = compiler.compile(packageSource, { officialBuild: true }); + var compileResult = compiler.compile(packageSource, + { officialBuild: true }); if (buildmessage.jobHasMessages()) { process.stderr.write("Error compiling unipackage:" + item + "\n"); return; @@ -1909,31 +1910,45 @@ main.registerCommand({ // If there is no old version, then we need to publish this package. if (!oldVersion) { - toPublish[item] = {source: packageSource, unipackage: compileResult}; + toPublish[item] = {source: packageSource, + compileResult: compileResult}; return; } else { - // Now we need to check if the compiler inputs hash matches up. If - // it doesn't, we need to update the version *and* republish. This - // will constitute an error -- log a message, and mark the error - // so we exit before trying to publish the release. - var myCompilerInputsHash = compileResult.unipackage.getBuildIdentifier({ - relativeTo: packageSource.sourceRoot - }); - if (myCompilerInputsHash === oldVersion.compilerInputsHash) { - // Cool, everything is exactly the same. We are done here. - return; + var existingBuild = catalog.getBuildWithArchesString( + item, oldVersion, + compileResult.unipackage.architecturesString()); + // If the version number mentioned in package.js exists, but + // there's no build of this architecture, then either the old + // version was only semi-published, or you've added some + // platform-specific dependencies but haven't bumped the + // version number yet; either way, you should probably bump + // the version number. + var somethingChanged = !existingBuild; + + if (!somethingChanged) { + // Bundle the build, just to get its hash. + // XXX this is redundant with the bundle build step that + // publishPackage will do later + var bundleBuildResult = packageClient.bundleBuild( + compileResult.unipackage); + if (bundleBuildResult.tarballHash !== + existingBuild.build.hash) { + somethingChanged = true; + } } - // The build ID of the old server record is not the same as - // the buildID that we have on disk. This means something has - // changed -- maybe our source files, or a buildId of one of - // our build-time dependencies. There might be a false - // positive here (for example, we added some comments to a - // package.js file somewhere), but, for now, we would rather - // err on the side of catching this issue and forcing a more - // thorough check. - buildmessage.error("Something changed in package " + item + "." + - " Please upgrade version number. \n"); + if (somethingChanged) { + // The build ID of the old server record is not the same as + // the buildID that we have on disk. This means something + // has changed -- maybe our source files, or a buildId of + // one of our build-time dependencies. There might be a + // false positive here (for example, we added some comments + // to a package.js file somewhere), but, for now, we would + // rather err on the side of catching this issue and forcing + // a more thorough check. + buildmessage.error("Something changed in package " + item + + ". Please upgrade version number. \n"); + } } }); } @@ -1960,7 +1975,7 @@ main.registerCommand({ // checks around this. var pub = packageClient.publishPackage( prebuilt.source, - prebuilt.unipackage, + prebuilt.compileResult, conn, opts); diff --git a/tools/package-client.js b/tools/package-client.js index 4429606324..d9e02cfddc 100644 --- a/tools/package-client.js +++ b/tools/package-client.js @@ -386,13 +386,13 @@ var uploadTarball = function (putUrl, tarball) { exports.uploadTarball = uploadTarball; -var bundleBuild = function (unipackage, packageDir) { +var bundleBuild = function (unipackage) { var tempDir = files.mkdtemp('build-package-'); var packageTarName = unipackage.name + '-' + unipackage.version + '-' + - unipackage.architectures().join('+'); + unipackage.architecturesString(); var tarInputDir = path.join(tempDir, packageTarName); - files.cp_r(path.join(packageDir, '.build.' + unipackage.name), tarInputDir); + unipackage.saveToPath(tarInputDir); // Don't upload buildinfo.json. It's only of interest locally (for // example, it contains a watchset with local paths). @@ -413,7 +413,7 @@ var bundleBuild = function (unipackage, packageDir) { exports.bundleBuild = bundleBuild; -var createAndPublishBuiltPackage = function (conn, unipackage, packageDir) { +var createAndPublishBuiltPackage = function (conn, unipackage) { process.stdout.write('Creating package build...\n'); var uploadInfo = conn.call('createPackageBuild', { packageName: unipackage.name, @@ -421,7 +421,7 @@ var createAndPublishBuiltPackage = function (conn, unipackage, packageDir) { architecture: unipackage.architectures().join('+') }); - var bundleResult = bundleBuild(unipackage, packageDir); + var bundleResult = bundleBuild(unipackage); process.stdout.write('Uploading build...\n'); uploadTarball(uploadInfo.uploadUrl, @@ -538,10 +538,6 @@ exports.publishPackage = function (packageSource, compileResult, conn, options) sources, packageSource.sourceRoot); - var compilerInputsHash = compileResult.unipackage.getBuildIdentifier({ - relativeTo: packageSource.sourceRoot - }); - // Create the package. Check that the metadata exists. if (options.new) { process.stdout.write('Creating package...\n'); @@ -557,8 +553,7 @@ exports.publishPackage = function (packageSource, compileResult, conn, options) description: packageSource.metadata.summary, earliestCompatibleVersion: packageSource.earliestCompatibleVersion, containsPlugins: packageSource.containsPlugins(), - dependencies: packageSource.getDependencyMetadata(), - compilerInputsHash: compilerInputsHash + dependencies: packageSource.getDependencyMetadata() }; var uploadInfo = conn.call('createPackageVersion', uploadRec); @@ -574,8 +569,6 @@ exports.publishPackage = function (packageSource, compileResult, conn, options) conn.call('publishPackageVersion', uploadInfo.uploadToken, bundleResult.tarballHash); - createAndPublishBuiltPackage(conn, compileResult.unipackage, - packageSource.sourceRoot); + createAndPublishBuiltPackage(conn, compileResult.unipackage); return 0; - }; diff --git a/tools/unipackage.js b/tools/unipackage.js index 896b861b32..f55667ced9 100644 --- a/tools/unipackage.js +++ b/tools/unipackage.js @@ -302,6 +302,11 @@ _.extend(Unipackage.prototype, { return _.uniq(_.pluck(self.builds, 'arch').concat(self._toolArchitectures())).sort(); }, + architecturesString: function () { + var self = this; + return self.architectures().join('+'); + }, + _toolArchitectures: function () { var self = this; var toolArches = _.pluck(self.toolsOnDisk, 'arch');