Skip to content

elmPackages.elm-i18next-gen: init at 1.1.0#320127

Closed
ilyakooo0 wants to merge 1 commit into
NixOS:masterfrom
ilyakooo0:iko/init-elm-i18next-gen-again
Closed

elmPackages.elm-i18next-gen: init at 1.1.0#320127
ilyakooo0 wants to merge 1 commit into
NixOS:masterfrom
ilyakooo0:iko/init-elm-i18next-gen-again

Conversation

@ilyakooo0

Copy link
Copy Markdown
Contributor

Description of changes

Reimplementation of #267454

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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

@ofborg ofborg Bot added the 8.has: package (new) This PR adds a new package label Jun 15, 2024
@ofborg ofborg Bot requested review from PedroHLC, jali-clarke, turboMaCk and zupo June 15, 2024 20:29
@ofborg ofborg 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. labels Jun 15, 2024
@turboMaCk

Copy link
Copy Markdown
Member

Thanks for PR.

Unfortunetely this PR seems to cause some regressions:

3 packages failed to build:
elmPackages.elm-land elmPackages.elm-pages elmPackages.elm-verify-examples

Which is not directly related to your changes but to the updates of the packages I believe. I think we will need to resolve these.

@ilyakooo0

Copy link
Copy Markdown
Contributor Author

@turboMaCk How would I go about fixing the regression?

@PedroHLC

PedroHLC commented Jun 16, 2024

Copy link
Copy Markdown
Member

How would I go about fixing the regression?

It's not technically your fault, node2nix is not only adding your package to node-packages.nix, but also bumping all of them.

Instead of fixing it, try to avoid it by doing a clean commit:

  1. hard-reset your branch to master
  2. without any modification, run generate-node-packages.sh
  3. commit the changes (let's call this commit "elm-node bump")
  4. add the package; re-run generate-node-packages.sh
  5. commit as "elmPackages.elm-i18next-gen: init at 1.1.0"
  6. git rebase -i master, change the "elm-node bump" from "pick" to "drop", :wq
  7. solve the conflicts manually (gonna pray that there is none to help you)
  8. git add -u && git rebase --continue && git push --force

@turboMaCk

Copy link
Copy Markdown
Member

It's not technically your fault, node2nix is not only adding your package to node-packages.nix, but also bumping all of them.

yes. And the update here is a problem. I think we should look into why newer version of these fails and how can we patch it. I knew this is a problem before your PR. I just didn't have time to look into it yet. It might be simple or it might be complex. That's hard to say without proper investigation into what the problem is.

@PedroHLC PedroHLC left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ilyakooo0 any interest in resuming this one?

@ilyakooo0 ilyakooo0 force-pushed the iko/init-elm-i18next-gen-again branch from 9bb8c39 to f3d8f3a Compare September 4, 2024 21:41
@ofborg ofborg Bot requested a review from PedroHLC September 4, 2024 22:51
@turboMaCk

turboMaCk commented Sep 5, 2024

Copy link
Copy Markdown
Member

there is an interest but unfortunately I'm extremely busy myself these days.

Because of coupling of all node packages via node2nix in here it would be best to first do the upgrade of existing packages and resolve all issues which will raise with it. Then we can add new package easily without causing regressions.

@ilyakooo0

Copy link
Copy Markdown
Contributor Author

I'll look into it today

@PedroHLC

PedroHLC commented Sep 5, 2024

Copy link
Copy Markdown
Member

Result of nixpkgs-review pr 320127 run on x86_64-linux 1

3 packages failed to build:
  • elmPackages.elm-land
  • elmPackages.elm-pages
  • elmPackages.elm-verify-examples
12 packages built:
  • elmPackages.create-elm-app
  • elmPackages.elm-analyse
  • elmPackages.elm-coverage
  • elmPackages.elm-doc-preview
  • elmPackages.elm-git-install
  • elmPackages.elm-graphql
  • elmPackages.elm-i18next-gen
  • elmPackages.elm-language-server
  • elmPackages.elm-live
  • elmPackages.elm-review
  • elmPackages.elm-spa
  • elmPackages.elm-upgrade

@turboMaCk

Copy link
Copy Markdown
Member

@zupo Are you willing to look into the elm-land update?

@zupo

zupo commented Sep 18, 2024

Copy link
Copy Markdown
Contributor

@zupo Are you willing to look into the elm-land update?

Gave it a try, got it to reproduce locally, but this is way above my pay grade, sorry :(

@turboMaCk

Copy link
Copy Markdown
Member

Ok I had a quick look. It seems that both elm-land and elm-pages started depending on packages that are pulling native dependencies like esbuild. It would be probably possible to patch these away but then there is also this bug npm/cli#4828

I'm myself not really invested in these two packages - never used either of them. So I would consider simply marking them as broken.

@zupo

zupo commented Sep 19, 2024

Copy link
Copy Markdown
Contributor

If you don't mind waiting, I will have another look later next week. I use both elm-land and elm-pages. That said, it's perfectly fine if you wanna mark them as broken for now and move on.

@turboMaCk

Copy link
Copy Markdown
Member

If you want I can give you some hints on the possible direction. I think we can wait. This has been delayed for a while but I don't like the idea of dropping the packages.

@zupo

zupo commented Sep 26, 2024

Copy link
Copy Markdown
Contributor

@domenkozar and I paired up and got a PoC of a fix sort of working. But it's getting too late to wrap it up. Here's the work in progress diff: e6201cb

@domenkozar domenkozar mentioned this pull request Sep 27, 2024
@domenkozar

Copy link
Copy Markdown
Member

#344866

@turboMaCk

Copy link
Copy Markdown
Member

@ilyakooo0 Complications around regression are now resolved. Can you re-open new PR based on current master? If you do please tag me. I will make sure it will get timely merge. Thanks for the patience.

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

Labels

8.has: package (new) This PR adds a new package 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.

5 participants