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

vsix file is not reproducible #1139

Open
stevedlawrence opened this issue Dec 5, 2024 · 9 comments
Open

vsix file is not reproducible #1139

stevedlawrence opened this issue Dec 5, 2024 · 9 comments
Labels
build Issues related to building of the code dependencies Pull requests that update a dependency file typescript

Comments

@stevedlawrence
Copy link
Member

The .vsix file generated by vsce package contains timestamps that prevent it from being easily reproducible. It looks like everything inside the zip is reproducible, so the vsix file itself is likely the only issue. I believe all the daffodil and daffodil-sbt releases are reproducible, so this is the last remaining piece that when fixed will allow us to switch to tag/CI based automated release building/signing, which should make the release process significantly easier.

I've done some digging and it looks like the issue is vsce includes the mtimes of the files in the zip/vsix file.

I've tried adding a new script in build/yarn-scripts.ts that is run just before we package the vsix file to change all the timestamps to SOURCE_DATE_EPOCH if defined. This works for most files included in the vsix file, but there are a couple that vsce generates during the package process that I don't think we can modify like that. So this likely is not a complete solution.

Another option is to create a PR to vscode-vsce to update it to use SOURCE_DATE_EPOCH when creating the vsix. I've skimmed vscode-vsce and think it could probably done with just a few lines, but I'm not all that familiar with node.js or how to test those kinds of changes. Also, even if we did make the changes, it requires getting a PR merged and updating the dependency to it. The first is probably doable, but daffodil-vscode depends on a pretty old version of vscode-vsce--is there anything blocking us from updating to a newer version if one were released with this kind of fix?

An alternative approach might be to just add a post processing script that unzips the contexts of the vsix file to a temp directory, modifies all the timestamps to SOURCE_DATE_EPOCH, and then rezips it. That's a bit of a pain, but doesn't have any dependency concerns.

@stevedlawrence
Copy link
Member Author

For reference, this patch to vscode-vsce seems to fix it so it can create reproducible zips by sorting the files and using SOURCE_DATE_EPOCH if defined:

diff --git a/src/package.ts b/src/package.ts
index d27b71c..9416b45 100644
--- a/src/package.ts
+++ b/src/package.ts
@@ -1800,13 +1800,21 @@ function writeVsix(files: IFile[], packagePath: string): Promise<void> {
 			() =>
 				new Promise((c, e) => {
 					const zip = new yazl.ZipFile();
-					files.forEach(f =>
+
+					const sde = process.env.SOURCE_DATE_EPOCH;
+					const epoch = sde ? parseInt(sde) : undefined;
+					const mtime = epoch ? new Date(epoch * 1000) : undefined;
+
+					files.sort((a, b) => a.path.localeCompare(b.path)).forEach(f => {
+						let options = {
+							mode: f.mode,
+						} as any;
+						if (mtime) options.mtime = mtime;
+
 						isInMemoryFile(f)
-							? zip.addBuffer(typeof f.contents === 'string' ? Buffer.from(f.contents, 'utf8') : f.contents, f.path, {
-								mode: f.mode,
-							})
-							: zip.addFile(f.localPath, f.path, { mode: f.mode })
-					);
+							? zip.addBuffer(typeof f.contents === 'string' ? Buffer.from(f.contents, 'utf8') : f.contents, f.path, options)
+							: zip.addFile(f.localPath, f.path, options)
+					});
 					zip.end();
 
 					const zipStream = fs.createWriteStream(packagePath);

I imagine there's a cleaner syntax to deal with the undefined stuff, but otherwise this seems fairly straightfoward and maybe something vscode-vsce would be willing to accept.

I think the question is if we did try to upstream this to vscode-vsce, would daffodil-vscode be able to update to the newer version, or is there a reason we need to stick with an older version.

@shanedell
Copy link
Contributor

@stevedlawrence I don't think there is necessarily a reason for not upgrading vsce other than possibly needing to update other dependencies in order for it to work? I just tested running yarn package with the latest version of vsce and it seemed to build fine. So this might be a good PR to add to the vscode-vsce repository

@scholarsmate
Copy link
Contributor

Thanks for looking into this @stevedlawrence, and I think regardless what we decide for this project, it's worth opening an upstream PR for vscode-vsce as there is certainly value to be had in terms of maintaining reproducibility for projects beyond Daffodil.

As for us upgrading to a newer version... what we need to consider is how it affects our minimum supported version of VSCode. Now that 1.4.0 has been released, we can start to take a look at where we need to be in terms of minimum requirements for Node, Java/Scala, and VSCode. This is a handy reference for Node <-> VSCode https://github.com/ewanharris/vscode-versions requirements.

In the meantime, the unpack and repack of the .vsix, while a pain, is something in our immediate realm of control, with little risk, and gets us the reproducibility property that we're seeking.

@shanedell
Copy link
Contributor

@stevedlawrence The syntax and way you went about the change looks fine to me. Feel free to open a PR to the vscode-vsce repo with your changes, I can also do this based on your decision, just let me know.

@scholarsmate
Copy link
Contributor

@shanedell does the .vsix still work fine on VSCode 1.82.0, our minimum required version? That's my concern. We want to be able to support about a year's worth of VSCode releases.

Ref: https://github.com/apache/daffodil-vscode/wiki/Apache-Daffodil™-Extension-for-Visual-Studio-Code:-v1.4.0#prerequisites

@stevedlawrence
Copy link
Member Author

I've created a PR to update vscode-vsce: microsoft/vscode-vsce#1100

I did notice vscode-vsce README says they require Node.js 20+. I don't know if you're also trying to maintain support for older versions of nodejs, or if vsce really does require a newer version of node or if that's just all they officially support.

Also, I built a vsix with the the current vcse 2.2.0 dependency and with the latest version with my patch, and contents were basically the same. The only functional difference that I see is the new extension.vsixmanifest file adds these two new properties:

<Property Id="Microsoft.VisualStudio.Code.EnabledApiProposals" Value="" />
<Property Id="Microsoft.VisualStudio.Code.ExecutesCode" Value="true" />

I imagine older versions of vscode would just ignore these if they don't support them.

One side note is that org.apache.daffodil.daffodil-debugger-1.4.0.jar is not exactly the same in the two .vsix files I built, so there might be another source of non-reproducibility to investigate. I don't think I saw that in previous tests, so it might not be as simple as a timestamp.

@stevedlawrence
Copy link
Member Author

Just a little more investigation, the differences in the org.apache.daffodil.daffodil-debugger-1.4.0.jar are in the JAXB generated classes (e.g. tdml/DFDLBaseType, tdml/DFDLChoiceType, etc.). Disassembling and diffing those .class files shows they have the same members and methods, but defined in a different order. I guess JAXB output is not entirely deterministic? This sounds familiar but I thought that was fixed?

@scholarsmate
Copy link
Contributor

@stevedlawrence thanks for sending that PR to vsce. It was good to see that someone else desired the same reproducibility property and opened an issue for it (though it got closed as out of scope). I asked that they reconsider and with your simple, low risk patch, I hope they decide to adopt the reproducibility property and merge in your PR.

@scholarsmate scholarsmate added dependencies Pull requests that update a dependency file build Issues related to building of the code typescript labels Dec 9, 2024
@stevedlawrence
Copy link
Member Author

The change to VSCode has been merged, looks like it will be in the next 3.2.2 release. I think it's still TBD whether or not newer versions of VSCode and its dependencies (e.g. node.js >= 20) breaks minimum version support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues related to building of the code dependencies Pull requests that update a dependency file typescript
Projects
None yet
Development

No branches or pull requests

3 participants