Skip to content

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

Merged
turboMaCk merged 1 commit intoNixOS:masterfrom
Kranzes:elm-land
Feb 18, 2025
Merged

elm-land: Rewrite and migrate from elmPackages to pkgs/by-name#382369
turboMaCk merged 1 commit intoNixOS:masterfrom
Kranzes:elm-land

Conversation

@Kranzes
Copy link
Copy Markdown
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 Kranzes requested review from domenkozar and zupo February 15, 2025 17:50
@Kranzes Kranzes mentioned this pull request Feb 15, 2025
13 tasks
@Kranzes Kranzes marked this pull request as draft February 15, 2025 17:54
@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. labels Feb 15, 2025
@Kranzes
Copy link
Copy Markdown
Member Author

Kranzes commented Feb 15, 2025

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

Comment thread pkgs/development/compilers/elm/packages/node/default.nix Outdated
Comment thread pkgs/by-name/el/elm-land/package.nix Outdated
@github-actions github-actions Bot added 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. and removed 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. labels Feb 15, 2025
Comment thread pkgs/by-name/el/elm-land/package.nix Outdated
@Kranzes Kranzes force-pushed the elm-land branch 4 times, most recently from 3036ec6 to 73fe88c Compare February 15, 2025 19:31
Comment thread .editorconfig Outdated
@zupo
Copy link
Copy Markdown
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
Copy Markdown
Member

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

@zupo
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

zupo commented Feb 18, 2025

Verified the latest commit works on macOS!

@turboMaCk
Copy link
Copy Markdown
Member

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

Signed-off-by: Ilan Joselevich <personal@ilanjoselevich.com>
@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@zupo
Copy link
Copy Markdown
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!

@turboMaCk
Copy link
Copy Markdown
Member

I think the danger of regression was in this case mitigated by decoupling the package from unified node2nix packages. Any update of this will from now on be intentional.

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

Labels

10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants