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

rt: 4.4.4 -> 5.0.1 #120926

Merged
merged 13 commits into from
May 7, 2021
Merged

rt: 4.4.4 -> 5.0.1 #120926

merged 13 commits into from
May 7, 2021

Conversation

ajs124
Copy link
Member

@ajs124 ajs124 commented Apr 27, 2021

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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.

@grahamc
Copy link
Member

grahamc commented Apr 27, 2021

I'm quite skeptical about these patches you've applied, they don't seem to be relevant to NixOS as a whole. What's up?

@dasJ
Copy link
Member

dasJ commented Apr 27, 2021

@ajs124 Would you mind adding yourself as a maintainer?

@ajs124
Copy link
Member Author

ajs124 commented Apr 27, 2021

I'm quite skeptical about these patches you've applied, they don't seem to be relevant to NixOS as a whole. What's up?

dont-check-users_groups.patch is needed for that script to work, most of the others can probably be dropped. Although I'm not sure why you'd want your rt to advertise it's version number on the login page or in every e-mail.

@grahamc
Copy link
Member

grahamc commented Apr 27, 2021

What is the user/group patch fixing, is that run during build time?

@dasJ
Copy link
Member

dasJ commented Apr 27, 2021

Yes, and there is no way to turn it off

@grahamc
Copy link
Member

grahamc commented Apr 27, 2021

:shipit:

Copy link
Member

@stigtsp stigtsp left a comment

Choose a reason for hiding this comment

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

The patches included in this PR needs more details:
What are their purpose? Are they found in other distributions?

@dasJ
Copy link
Member

dasJ commented Apr 27, 2021

They are probably not found in other distributions since I created some of them ;) Apart from the users-groups patch (which is needed to get it to "build" in the sandbox) the rest is to prevent RT from leaking its version all over the internet. May be a good choice so people don't find vulns for that specific version too easily

@@ -82,18 +136,27 @@ stdenv.mkDerivation rec {
"--enable-graphviz"
"--enable-gd"
"--enable-gpg"
"--with-db-type=SQLite"
"--enable-smime"
"--with-db-type=Pg"
Copy link
Member

@stigtsp stigtsp Apr 27, 2021

Choose a reason for hiding this comment

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

Does this mean that SQLite support is dropped? Does this have implications for existing installs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm. I'd be surprised if anyone is actually using this from nixpkgs right now. I see you fixed the build in 3e50e26. It has broken since. Trying to run the build from that commit doesn't work either, because the RT_SiteConfig.pm isn't writable.

Copy link
Member

@stigtsp stigtsp May 6, 2021

Choose a reason for hiding this comment

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

Good points. The db-type change seems ok.

@stigtsp
Copy link
Member

stigtsp commented Apr 27, 2021

[..] the rest is to prevent RT from leaking its version all over the internet. May be a good choice so people don't find vulns for that specific version too easily

Thanks for providing more info, and for the work on the patches. :) I'd think patches that remove the RT version from misc places are more suited for upstream, imho.

@stigtsp
Copy link
Member

stigtsp commented May 6, 2021

LGTM apart from the version number stripping patches.

@ajs124
Copy link
Member Author

ajs124 commented May 7, 2021

Dropped the two version-stripping patches.
If there are no further objections, I'd merge this soonish, as ZHF (#122042) has started and this currently fails on master.

Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

LGTM, but not really familiar with perl

@stigtsp is happy with it

https://github.com/NixOS/nixpkgs/pull/120926

25 packages built:
perl530Packages.AlienBaseModuleBuild perl530Packages.AlienLibGumbo perl530Packages.CryptX509 perl530Packages.HTMLFormatExternal perl530Packages.HTMLGumbo perl530Packages.HTTPHeadersActionPack perl530Packages.MooXTypeTiny perl530Packages.PathDispatcher perl530Packages.ShellConfigGenerate perl530Packages.ShellGuess perl530Packages.TextWordDiff perl530Packages.WebMachine perl532Packages.AlienBaseModuleBuild perl532Packages.AlienLibGumbo perl532Packages.CryptX509 perl532Packages.HTMLFormatExternal perl532Packages.HTMLGumbo perl532Packages.HTTPHeadersActionPack perl532Packages.MooXTypeTiny perl532Packages.PathDispatcher perl532Packages.ShellConfigGenerate perl532Packages.ShellGuess perl532Packages.TextWordDiff perl532Packages.WebMachine rt

@jonringer jonringer merged commit 590ff31 into NixOS:master May 7, 2021
@ajs124 ajs124 deleted the fix-and-update/rt branch May 7, 2021 17:35
@sbourdeauducq
Copy link
Contributor

Is there a NixOS module available for this? If not, could someone share the relevant parts of their configuration.nix to use this with a web server (preferably nginx)?

@ajs124
Copy link
Member Author

ajs124 commented May 8, 2021

@sbourdeauducq: https://gist.github.com/ajs124/ff04ab14435908d914cf5cedbc56a52e

It's probably not generic enough + I removed the postgres config, because that uses our own postgres module. Although scrolling over it, I forgot to remove the sandbox and apparmor parameters for the systemd services.
Maybe it will be useful to you, but I don't really plan on putting this into nixpkgs.

@sbourdeauducq
Copy link
Contributor

@ajs124 thank you!

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.

6 participants