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.

Sign up for GitHub

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

Jump to bottom

vscode: fix libdbusmenu on kde #210593

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

vscode: fix libdbusmenu on kde #210593

wants to merge 1 commit into from
+3 −0

Conversation

pasqui23
Copy link
Contributor

@pasqui23 pasqui23 commented Jan 13, 2023 •
edited

Description of changes

Fix #65680 and also a bug that broke attaching the node debugger to an already running process

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.05 Release Notes (or backporting 22.11 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@turion
Copy link
Contributor

turion commented Jan 14, 2023

Thanks :) can you fix the two typos in the commit messages?

@ofborg test vscode

@turion
Copy link
Contributor

turion commented Jan 14, 2023

To be honest I don't understand how/why this fix works, but that's probably just a lack of knowledge on my side. If you could briefly explain why this works and the previous version didn't, that would be great. Or else another reviewer could approve before I merge.

Also, can you fix the typos in the PR title?

@pasqui23 pasqui23 changed the title Vscode: fix libdbudmenu on kde, fix attach debuggehttps://github.com/NixOS/nixpkgs/pullsr to node process Vscode: fix libdbudmenu on kde, fix attach debugger to node process Jan 14, 2023
@ofborg ofborg bot requested a review from turion January 18, 2023 00:17
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/1819

@turion turion changed the title Vscode: fix libdbudmenu on kde, fix attach debugger to node process vscode: fix libdbusmenu on kde, fix attach debugger to node process Feb 13, 2023
@turion
Copy link
Contributor

turion commented Feb 13, 2023

Can you fix the typos in the commit messages?

vscode: fixing libdbudmenu on kde
vscode: fix attach debbugger to node process functionality

libdbudmenu -> libdbusmenu

debbugger -> debugger

This is important so one can later quickly search for commits using git log -S.

Otherwise, this PR is looking good :)

@turion
Copy link
Contributor

turion commented Feb 13, 2023 •
edited by SuperSandro2000

@ofborg test vscode

@turion
Copy link
Contributor

turion commented Feb 13, 2023 •
edited by SuperSandro2000

@ofborg test vscodium

@turion
Copy link
Contributor

turion commented Feb 13, 2023

Why are all the tests broken?

@pasqui23
Copy link
Contributor Author

Why are all the tests broken?

Seems that vscode.tests just alias to pkgs.test, and I think that at any given time a number of pkg are always broken, especially on master

@turion
Copy link
Contributor

turion commented Feb 13, 2023

Seems that vscode.tests just alias to pkgs.test, and I think that at any given time a number of pkg are always broken, especially on master

Oh wow, it seems like that's a bug I introduced 3 years ago and noone noticed...? But maybe it's also got to do with the unfree license somehow...

@pasqui23
Copy link
Contributor Author

Seems that vscode.tests just alias to pkgs.test, and I think that at any given time a number of pkg are always broken, especially on master

Oh wow, it seems like that's a bug I introduced 3 years ago and noone noticed...? But maybe it's also got to do with the unfree license somehow...

When did you introduce it? Maybe I can reverse it.

@pasqui23
Copy link
Contributor Author

Should I remove the tests passthru? It seems in pkgs.tests there is nothing for vscode.

@bobby285271
Copy link
Member

bobby285271 commented Feb 14, 2023 •
edited

@ofborg test vscodium.xorg

@bobby285271 bobby285271 mentioned this pull request Feb 14, 2023
Open
@bobby285271
Copy link
Member

bobby285271 commented Feb 14, 2023 •
edited

We test vscodium on vscode changes since generic.nix is shared and we only have nixosTests.vscodium. It should be fine for the wayland test to fail (not regression), but it looks like this PR breaks the xorg test. 🙃

Not sure if it is a good idea to force use the portal, since the test provides a scenario where the file chooser may no longer open with the changes (I guess?) 🤔

@turion
Copy link
Contributor

turion commented Feb 14, 2023

It seems in pkgs.tests there is nothing for vscode.

I believe the idea is that there should be no tests for vscode because we can't run unfree software on CI, but the tests for vscodium have to remain. I'll fix that separately.

@turion
Copy link
Contributor

turion commented Feb 14, 2023

See #216278

@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict labels Mar 19, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@superherointj superherointj marked this pull request as draft April 16, 2024 19:10
@amaxine amaxine removed their request for review April 27, 2024 17:55
@pasqui23 pasqui23 marked this pull request as ready for review June 4, 2024 19:41
@ofborg ofborg bot requested review from Enzime and LudovicoPiero June 4, 2024 21:30
SuperSandro2000
SuperSandro2000 approved these changes Jun 5, 2024
View reviewed changes
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

assuming test runs through

SuperSandro2000
SuperSandro2000 reviewed Jun 5, 2024
View reviewed changes
pkgs/applications/editors/vscode/generic.nix Outdated
@@ -225,6 +228,10 @@ in
--replace "/bin/bash" "${bash}/bin/bash"
rm -rf "$packed"

# this fixes "Node Debugger:Attach to process"
substituteInPlace resources/app/extensions/ms-vscode.js-debug/src/extension.js \
--replace-fail "/bin/ps" "${procps}/bin/ps"
Copy link
Member

Choose a reason for hiding this comment

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

That no longer matches and I can't find it in https://github.com/search?q=repo%3Amicrosoft%2Fvscode-js-debug%20%2Fbin%2Fps&type=code

@SuperSandro2000 SuperSandro2000 changed the title vscode: fix libdbusmenu on kde, fix attach debugger to node process vscode: fix libdbusmenu on kde Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bobby285271 bobby285271 Awaiting requested review from bobby285271

@Synthetica9 Synthetica9 Awaiting requested review from Synthetica9

@eadwu eadwu Awaiting requested review from eadwu

@turion turion Awaiting requested review from turion

@Enzime Enzime Awaiting requested review from Enzime

@LudovicoPiero LudovicoPiero Awaiting requested review from LudovicoPiero

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

Assignees
No one assigned
Labels
6.topic: vscode 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Integrate vscode with libdbusmenu
6 participants
@pasqui23 @turion @nixos-discourse @bobby285271 @SuperSandro2000 @wegank