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

elm-land: Rewrite and migrate from elmPackages to pkgs/by-name #382369

Merged
merged 1 commit into from
Feb 18, 2025

Conversation

Kranzes
Copy link
Member

@Kranzes Kranzes commented Feb 15, 2025

Supersedes: #382339

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.

@Kranzes
Copy link
Member Author

Kranzes commented Feb 15, 2025

Should I add an alias from pkgs.elmPackages.elm-land to pkgs.elm-land?

@Kranzes Kranzes force-pushed the elm-land branch 4 times, most recently from 3036ec6 to 73fe88c Compare February 15, 2025 19:31
@zupo
Copy link
Contributor

zupo commented Feb 16, 2025

Doesn't seem to work on nix-darwin.

../result/bin/elm-land server does say Elm Land (v0.20.1) is ready at http://localhost:1234, but nothing happens in my browser when I do to this address.

Uploading Screenshot 2025-02-16 at 11.14.20.png…

@turboMaCk
Copy link
Member

@zupo what does lsof -i :1234 returns in that state?

@zupo
Copy link
Contributor

zupo commented Feb 16, 2025

lsof -i :1234

COMMAND     PID USER   FD   TYPE             DEVICE SIZE/OFF NODE NAME
node       7231 zupo   13u  IPv4 0xf221e680ffb1b9d6      0t0  TCP *:search-agent (LISTEN)
ssh       87800 zupo    5u  IPv6 0x8dc076cf21014dda      0t0  TCP localhost:search-agent (LISTEN)
ssh       87800 zupo    6u  IPv4 0x16cc71eabb3ccfdf      0t0  TCP localhost:search-agent (LISTEN)
Google    99313 zupo   38u  IPv6 0x731ae9acde4f50c3      0t0  TCP localhost:49474->localhost:search-agent (CLOSE_WAIT)

elm-land build seems to work fine.

@turboMaCk
Copy link
Member

turboMaCk commented Feb 16, 2025

you can see that node is attached to the port from pid 7231 that will probably be the elm-land process. But chrome is stuck in CLOSE_WAIT which I believe suggests the server closed the connection but chrome did not. What is a bit sus is that server is on ipv4 while chrome is connecting via ipv6. But I'm not an expert on networking especially in macos - But my guess is that problem is not with the package.

if you have non nixified version running locally you can compare what that does. It might be attaching to the port using ipv6 perhaps? I think you maybe can even fix this in your hostfile - maybe you're not binding localhost to ipv4 address. I would also definitely try accessing http://127.0.0.1:1234 directly.

@zupo
Copy link
Contributor

zupo commented Feb 16, 2025

@turboMaCk: thank you for taking the time to help me. It's definitely my machine/browser. curl works just fine. @Kranzes: sorry for the noise!

@Kranzes Kranzes force-pushed the elm-land branch 2 times, most recently from 4cef084 to 915e238 Compare February 18, 2025 09:49
@zupo
Copy link
Contributor

zupo commented Feb 18, 2025

Verified the latest commit works on macOS!

@turboMaCk
Copy link
Member

@Kranzes is there anything missing? I see this is still ina Draft.

@Kranzes Kranzes marked this pull request as ready for review February 18, 2025 14:20
@nix-owners nix-owners bot requested review from Mic92 and zowoq February 18, 2025 14:21
@Kranzes
Copy link
Member Author

Kranzes commented Feb 18, 2025

@Kranzes is there anything missing? I see this is still ina Draft.

Should be good now. @zupo wanted to write a test but I just tried to do that and I think it will be quite difficult because of the sandbox, it tries to fetch the packages list from the registry, which requires a network connection.

@turboMaCk
Copy link
Member

turboMaCk commented Feb 18, 2025

Honestly I'm OK not doing test for this case. But I'm not sure how exactly this regressed either so I don't even have an idea about what could make sense to test.

Plus this package is now completely decoupled from most things (apart from elm compiler).

Copy link
Member

@turboMaCk turboMaCk left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@PedroHLC PedroHLC left a comment

Choose a reason for hiding this comment

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

Diff LGTM.

@turboMaCk turboMaCk merged commit 490c009 into NixOS:master Feb 18, 2025
37 of 39 checks passed
@zupo
Copy link
Contributor

zupo commented Feb 19, 2025

@Kranzes is there anything missing? I see this is still ina Draft.

Should be good now. @zupo wanted to write a test but I just tried to do that and I think it will be quite difficult because of the sandbox, it tries to fetch the packages list from the registry, which requires a network connection.

I was dreading that 😱. In any case, having the test in a later PR is perfectly fine. Thanks again for fixing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants