Skip to content

copilot-language-server: add fhs version #393260

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

Merged
merged 1 commit into from
Apr 4, 2025

Conversation

arunoruto
Copy link
Contributor

@arunoruto arunoruto commented Mar 26, 2025

Solves issue #391730 and closes the PR #391748.

The main inspiration for the FHS environment were the vscode and zed-editor packages. An fhs passthru has been created and later referenced in pkgs/top-level/all-packages.nix.
I am not quite sure, if this is the correct of doing this, but it was the only reference I had.

This PR will provide an additional package copilot-language-server-fhs, which should solve the issue addressed in #391730. The "normal" package is kept, since it works on stable releases so far. It is possible, when 25.05 is released, that the normal package will break. The FHS package will then become the default one in the future.

I would be happy if someone could provide a better solution or a fix to the underlying problem. I guess it isn't packages correctly or the package needs additional information?

As for the PR #391748: using the auto patch hook breaks the binary and AFAIK it can't be fixed afterwards, which makes FHS environments the only solution.

If someone has the same problem as addressed in the issue, please test this PR and see if it solves your problem. Looking forward for your feedback.

For those who have a merge/commit bit: please only merge this PR after some testing has been done!

TODO:

  • It would be nice to have a versionCheckHook too, but I am not sure if I should introduce this in this PR or make a separate one.

Ping: @wattmto @drupol @brianmcgillion

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: 1 10.rebuild-linux: 1-10 labels Mar 26, 2025
@arunoruto
Copy link
Contributor Author

arunoruto commented Mar 26, 2025

Running both commands on my system via my own flake, not this PR:

../flake ❄ ❯ nix run .#copilot-language-server --impure -- --version
1.290.0

../flake ❄ ❯ nix run .#copilot-language-server-fhs --impure -- --version
1.290.0

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 393260


x86_64-linux

✅ 1 package built:
  • copilot-language-server-fhs

@arunoruto arunoruto force-pushed the copilot-language-server-fhs branch from 204de1b to e58f731 Compare March 26, 2025 01:54
@github-actions github-actions bot added 10.rebuild-darwin: 1 10.rebuild-darwin: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1 labels Mar 26, 2025
@nix-owners nix-owners bot requested a review from wattmto March 26, 2025 02:01
@arunoruto arunoruto requested a review from drupol March 26, 2025 13:20
@drupol
Copy link
Contributor

drupol commented Mar 27, 2025

You can add the versionCheckHook within this PR, in a new commit.

@brianmcgillion
Copy link
Contributor

@arunoruto thank you for all your efforts on this, really appreciated. can confirm that the -fhs version is working.

@arunoruto
Copy link
Contributor Author

You can add the versionCheckHook within this PR, in a new commit.

I add the versionCheckHook in the main package, not sure if it can be also added in the FHS env.

@drupol
Copy link
Contributor

drupol commented Mar 27, 2025

You can add the versionCheckHook within this PR, in a new commit.

I add the versionCheckHook in the main package, not sure if it can be also added in the FHS env.

Have you tested it before pushing? Do you see the output in the build log that it has been working successfully ?

@arunoruto arunoruto force-pushed the copilot-language-server-fhs branch from da93f50 to f75f5d2 Compare March 27, 2025 13:11
@drupol drupol marked this pull request as draft March 27, 2025 13:39
@arunoruto arunoruto force-pushed the copilot-language-server-fhs branch from f75f5d2 to 871f65b Compare March 27, 2025 14:10
@drupol drupol marked this pull request as ready for review March 27, 2025 14:27
@arunoruto arunoruto force-pushed the copilot-language-server-fhs branch from 871f65b to f85e984 Compare March 27, 2025 15:26
@arunoruto
Copy link
Contributor Author

arunoruto commented Mar 27, 2025

Minor changes:

  • Included a version attribute in buildFHSEnv
  • Replaced copilot-language-server argument name with package
  • Replaced the reference to the variableexecutableName with package.meta.mainProgram, so the fhs function only depends on the argument package.

I will probably make a new PR and try to include testers.testVersion instead of versionCheckHook, with the hope to catch weird behaviour with the binary as introduced by #391730:

Note

This is a check you add to passthru.tests which is mainly run by OfBorg, but not in Hydra. If you want a version check failure to block the build altogether, then versionCheckHook is the tool you’re looking for (and recommended for quick builds). The motivation for adding either of these checks would be

  • Catch dynamic linking errors and such and missing environment variables that should be added by wrapping.
  • Probable protection against accidentally building the wrong version, for example when using an “old” hash in a fixed-output derivation.

And thank you @drupol for reviewing the PR and for the tips!

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 393260


x86_64-linux

✅ 2 packages built:
  • copilot-language-server
  • copilot-language-server-fhs

@arunoruto arunoruto force-pushed the copilot-language-server-fhs branch from f85e984 to 05a75f2 Compare March 27, 2025 15:42
@arunoruto arunoruto added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Mar 28, 2025
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 2, 2025
@benley benley force-pushed the copilot-language-server-fhs branch from 05a75f2 to b81f7c0 Compare April 2, 2025 19:31
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 2, 2025
@benley
Copy link
Member

benley commented Apr 2, 2025

(rebased to pick up the latest version of the package from nixpkgs)

This seems to work. Is there any reason we shouldn't merge it?

@arunoruto
Copy link
Contributor Author

(rebased to pick up the latest version of the package from nixpkgs)

This seems to work. Is there any reason we shouldn't merge it?

Not that I am aware of. The passthru tests will be included in a separate PR since they would go beyond the topic of this one.

@drupol
Copy link
Contributor

drupol commented Apr 3, 2025

Hello,

I haven't merged this PR because I never packaged stuff related to fhs, this is why I'm waiting for someone else to review this PR.

Ping me in a couple of days, if nobody reviewed it, I'll merge it.

meta = package.meta // {
description =
package.meta.description
+ " (FHS-wrapped). Use this version if you have trouble with the normal one.";
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we mention here what kind of trouble like that executables cannot be executed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could I reference the issue #391730 in the text? And if so, as link or just the issue number?

Copy link
Member

@SuperSandro2000 SuperSandro2000 Apr 3, 2025

Choose a reason for hiding this comment

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

A comment in the form of If you receive an error like: Could not start dynamically linked executable plus a link to that issue would be super cool :)

Copy link
Contributor Author

@arunoruto arunoruto Apr 3, 2025

Choose a reason for hiding this comment

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

I added a disclaimer to open the package details for more information on when to use it, and added the error messages into longDescription. I wasn't sure if description supports markdown and it should be kept short, so I opted for the longDescription (hence the multiple force-pushes... check trice and push once 😂... still learning tho!).

@arunoruto arunoruto force-pushed the copilot-language-server-fhs branch from b81f7c0 to 05a75f2 Compare April 3, 2025 12:27
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 3, 2025
@arunoruto arunoruto force-pushed the copilot-language-server-fhs branch from 05a75f2 to 3547be8 Compare April 3, 2025 12:29
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 3, 2025
@arunoruto arunoruto force-pushed the copilot-language-server-fhs branch 2 times, most recently from c67346a to 69be34c Compare April 3, 2025 12:40
@arunoruto arunoruto force-pushed the copilot-language-server-fhs branch from 69be34c to cc402c5 Compare April 3, 2025 12:42
@benley benley merged commit 249b8ed into NixOS:master Apr 4, 2025
28 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants