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

Jump to bottom

Update: meguca, quicktemplate, statik, hydron -> Convert to buildGoModule #67666

Closed
wants to merge 10 commits into from
Closed

Update: meguca, quicktemplate, statik, hydron -> Convert to buildGoModule #67666

wants to merge 10 commits into from

Conversation

Chiiruno
Copy link
Contributor

@Chiiruno Chiiruno commented Aug 29, 2019 •
edited

Motivation for this change

Got meguca working again, so it's no longer marked as broken!
In the process, learned how we have a buildGoModule method now, much better than what was used previously!

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot requested a review from kalbasit August 29, 2019 01:12
@Chiiruno
Copy link
Contributor Author

cc @infinisil
the person ofborg CC'd is on vacation.

@Chiiruno
Copy link
Contributor Author

Chiiruno commented Aug 29, 2019 •
edited

Ran nix-shell -p nix-review --run "nix-review wip", only a few relatively unrelated failures in nodePackages, like usual.

teto
teto requested changes Aug 29, 2019
View reviewed changes
Copy link
Member

@teto teto left a comment

Choose a reason for hiding this comment

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

sslCertificate and postgresArgs options disappear, is that ok ? why ? anyway to achieve the same behavior otherwise

nixos/modules/services/web-servers/meguca.nix Show resolved Hide resolved
nixos/modules/services/web-servers/meguca.nix Show resolved Hide resolved
nixos/modules/services/web-servers/meguca.nix Show resolved Hide resolved
nixos/modules/services/web-servers/meguca.nix Outdated Show resolved Hide resolved
@Chiiruno
Copy link
Contributor Author

sslCertificate and postgresArgs options disappear, is that ok ? why ? anyway to achieve the same behavior otherwise

Yes, meguca doesn't support SSL in postgres, and postgresArgs is deprecated by the new config file, and is impossible to use command arguments now.

ciil
ciil approved these changes Aug 29, 2019
View reviewed changes
Copy link
Member

@ciil ciil left a comment

Choose a reason for hiding this comment

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

Just built it and it runs smoothly in a container.

nixos/modules/services/web-servers/meguca.nix Outdated Show resolved Hide resolved
@kalbasit
Copy link
Member

@GrahamcOfBorg build meguca quicktemplate statik hydron

@Chiiruno
Copy link
Contributor Author

Chiiruno commented Aug 29, 2019 •
edited

I noticed this in the darwin test, despite the test passing, meguca doesn't build due to opencv not being supported on darwin.

Cannot nix-instantiate `meguca' because:
error: while evaluating the attribute 'buildInputs' of the derivation 'meguca-6.3.0' at /private/var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/grahamc-ndndx-vm/pkgs/stdenv/generic/make-derivation.nix:191:11:
while evaluating the attribute 'handled' at /private/var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/grahamc-ndndx-vm/pkgs/stdenv/generic/check-meta.nix:251:7:
while evaluating 'handleEvalIssue' at /private/var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/grahamc-ndndx-vm/pkgs/stdenv/generic/check-meta.nix:147:38, called from /private/var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/grahamc-ndndx-vm/pkgs/stdenv/generic/check-meta.nix:252:14:
Package ‘opencv-2.4.13’ in /private/var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/grahamc-ndndx-vm/pkgs/development/libraries/opencv/default.nix:88 is not supported on ‘x86_64-darwin’, refusing to evaluate.
a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnsupportedSystem = true; }
in configuration.nix to override this.
b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnsupportedSystem = true; }
to ~/.config/nixpkgs/config.nix.

Is this something we should address?

@Chiiruno
Copy link
Contributor Author

Rebase due to #66585.

@Chiiruno
Copy link
Contributor Author

Chiiruno commented Sep 8, 2019

Depends on #68135
Will update PR with changes once that is merged.

infinisil
infinisil requested changes Sep 20, 2019
View reviewed changes
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

This could really use some splitting up into multiple commits, it's really hard to review like this

nixos/modules/services/web-servers/hydron.nix Outdated Show resolved Hide resolved
@Chiiruno
Copy link
Contributor Author

It was split up into multiple commits before, I wasn't expecting anyone else to review after I said this depends on #68135 , before fixing everything back up.
I just saved some temporary changes into a temporary commit since I had already reset to the master revision.
Anyway, sorry for any inconvenience.

@Chiiruno
Copy link
Contributor Author

Ready for review!

@Chiiruno Chiiruno force-pushed the dev/meguca branch 3 times, most recently from 6cadd03 to 8dabf6b Compare October 17, 2019 22:54
@Chiiruno
Copy link
Contributor Author

That should be a bit better.

@prusnak
Copy link
Member

prusnak commented May 30, 2020

Please rework your PR. It now has a merge conflict.

@Chiiruno
Copy link
Contributor Author

Outdated, v7 is on it's way.

@Chiiruno Chiiruno closed this Nov 18, 2020
@Chiiruno Chiiruno deleted the dev/meguca branch November 18, 2020 18:50
@Chiiruno Chiiruno mentioned this pull request Dec 28, 2020
Merged
10 tasks
This was referenced Dec 28, 2020
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teto teto teto requested changes

@infinisil infinisil infinisil requested changes

@ciil ciil ciil approved these changes

@kalbasit kalbasit Awaiting requested review from kalbasit

Assignees
No one assigned
Labels
2.status: merge conflict 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@Chiiruno @kalbasit @prusnak @teto @ciil @infinisil @ryantm @Madouura