Skip to content

vscode-extensions-update: init#389585

Merged
drupol merged 4 commits intomasterfrom
unknown repository
Mar 28, 2025
Merged

vscode-extensions-update: init#389585
drupol merged 4 commits intomasterfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Mar 13, 2025

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Mar 13, 2025
@drupol
Copy link
Contributor

drupol commented Mar 13, 2025

Hello,

I'm glad to see this issue being worked on. I haven't yet carefully check your script but here are a few suggestions:

  • Make sure that the updated extension is working with the current version of VSCode
  • Use Python

You can find a script I use regularly to update the VSCode extension here.

@ghost ghost marked this pull request as ready for review March 13, 2025 21:17
Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

I left a few minor comments.

I have more important questions now:

  1. I see you're doing a lot of processing to manually update the hash with the updated one. There are tools that do that already. Wouldn't it be better to use those tools instead of reinventing them?
  2. How do you handle extensions that have multiple hashes based on their system ? I have the feeling that this is not covered by the script.
  3. How is the script meant to be used? I don't really understand how to run it.

Thanks!

@github-actions github-actions bot added 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Mar 14, 2025
@github-actions github-actions bot added 6.topic: vscode A free and versatile code editor that supports almost every major programming language. and removed 6.topic: vscode A free and versatile code editor that supports almost every major programming language. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Mar 14, 2025
@nix-owners nix-owners bot requested a review from Zimmi48 March 14, 2025 16:30
@github-actions github-actions bot added 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Mar 14, 2025
@ghost ghost removed the request for review from Zimmi48 March 14, 2025 16:31
@drupol drupol changed the title vscode-extensions-update-script: init vscode-extensions-update: init Mar 15, 2025
@drupol

This comment was marked as resolved.

@nix-owners nix-owners bot requested a review from jraygauthier March 25, 2025 19:19
Copy link
Member

@jraygauthier jraygauthier left a comment

Choose a reason for hiding this comment

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

Haven't reviewed the update script itself but this is a really nice centralization of the update scripts, it removes a lot of complexity from packages. Good job.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we add the updater by default in this builder ?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, with the option to override.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Member

@JohnRTitor JohnRTitor Mar 26, 2025

Choose a reason for hiding this comment

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

Sorry, I meant that we should be able to override passthru.updateScript in extensions when we want.

inherit vscodeExtPublisher vscodeExtName vscodeExtUniqueId;

I think we can add it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fully possible since #390180

Copy link
Contributor

Choose a reason for hiding this comment

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

If I were to vote, I'd vote for updating them individually, without using any kind of json file.

Copy link
Member

Choose a reason for hiding this comment

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

To be honest, I am in favor of updating all of them in one go, periodically, while still allowing updates of individual extensions. vscode-extensions maintenance is the second worst on Nixpkgs, after kernel modules.

Provided we account for bugs like these of course. (#392786)

Copy link
Contributor

@drupol drupol Mar 28, 2025

Choose a reason for hiding this comment

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

How about merging this today, are you all OK with that? I think the script is great and is going to help a lot until we perhaps have another solution that you mentionned.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sadly I don't think this will be ever possible.

Copy link
Member

@JohnRTitor JohnRTitor Mar 28, 2025

Choose a reason for hiding this comment

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

Let's merge this today, and maybe try to reimplement the extensions structure to use a versions.json later. @emaryn you okay with that?

@wegank wegank added 12.approvals: 1 This PR was reviewed and approved by one person. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. labels Mar 26, 2025
@drupol drupol mentioned this pull request Mar 27, 2025
13 tasks
@github-actions github-actions bot added 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Mar 28, 2025
Copy link
Member

@JohnRTitor JohnRTitor left a comment

Choose a reason for hiding this comment

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

nixpkgs-vet check needs a fix perhaps

@drupol
Copy link
Contributor

drupol commented Mar 28, 2025

I'm thinking about something, would it be a good idea to add this script to all the extensions that have their own file (namely, those that are not defined in default.nix) ?

Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

All good

@drupol drupol merged commit f7357cf into NixOS:master Mar 28, 2025
24 of 27 checks passed
@drupol
Copy link
Contributor

drupol commented Mar 28, 2025

I'm trying to update the vscode extension, I added `meta.platforms set to:

platforms = [
  "x86_64-linux"
  "x86_64-darwin"
  "aarch64-linux"
  "aarch64-darwin"
];

The command I run is: vscode-extensions-update --platforms vscode-extensions.visualjj.visualjj

But it only update the x86_64-linux (my own local platform). Is there a way to update all the platforms available?

@drupol
Copy link
Contributor

drupol commented Mar 28, 2025

The command I run is: vscode-extensions-update --platforms vscode-extensions.visualjj.visualjj
But it only update the x86_64-linux (my own local platform). Is there a way to update all the platforms available?

Sorry, I wrote the wrong condition for the if statement. Also, to use this, nix needs to be able to build for other platforms.

There's no sorry needed here :) Do you plan to do an update PR?

If yes, WDYT about using the method execute_command in place of with subprocess.Popen(...) ?

@ghost ghost deleted the vscode-extensions-update-script branch March 28, 2025 17:45
@ghost ghost removed the request for review from jfchevrette April 11, 2025 19:41
@ghost ghost removed 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. 6.topic: vscode A free and versatile code editor that supports almost every major programming language. labels Jul 11, 2025
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.

6 participants