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

php80: mark as broken on 23.11 #236904

Closed
wants to merge 2 commits into from
Closed

Conversation

Atemu
Copy link
Member

@Atemu Atemu commented Jun 9, 2023

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.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.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jun 9, 2023
@Atemu Atemu mentioned this pull request Jun 9, 2023
8 tasks
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 labels Jun 9, 2023
@@ -337,6 +337,7 @@ let
maintainers = teams.php.members;
platforms = platforms.all;
outputsToInstall = [ "out" "dev" ];
broken = (versionOlder version "8.1") && (isInOldestRelease 2305); # Will automatically break php < 8.0 with 23.11
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed in #224505, I do not think this is correct. PHP itself is not broken.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not? I thought PHP 8.0 is going EOL and therefore no longer supported by us come November?

Copy link
Contributor

Choose a reason for hiding this comment

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

PHP 8.0 will be removed completely by then (preparing the PR to do that is in my todo list for next week).

What purpose does it serve to mark it as broken now?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wouldn't flat out remove in but rather mark it as broken/insecure. That way you can still opt into using dependant packages on your own risk.

The maintenance burden for a (marked broken) php80 should be close to nil.

Copy link
Contributor

Choose a reason for hiding this comment

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

At the same time, nixpkgs isn't a collection of unmaintained or insecure software. Sooner or later it will become a huge maintenance burden when some dependency is updated that the old version of PHP won't support. Should we then override and pin older versions of everything there?

So no, we'll drop the old version that will become EOL from unstable early in the release cycle so we have time to find out what's broken and attempt to patch it before the next release, or just mark it as broken by then during the ZHF process.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sooner or later it will become a huge maintenance burden when some dependency is updated that the old version of PHP won't support.

At that point, you'd simply remove that old, deprecated version of PHP. It will have likely been marked as broken/insecure for a while by then, so nobody can complain they weren't warned.

As you correctly stated, we're not a collection of unmaintained or insecure software. No support is guaranteed here. What I'm concerned about however is to keep those deprecated versions around until they actually cause real issues. As long as they do not cause real issues, they do not hurt us.

I wouldn't advocate for keeping it if there were no users but in this case we do have significant users within Nixpkgs. We're even using limesurvey right now for this year's community survey.

just mark it as broken by then during the ZHF process

This is effectively what my PR does. On branch-off, php80 will "automatically" be marked as broken. No 23.11 user can build a system with php80 without explicitly allowing the package.

Copy link
Contributor

Choose a reason for hiding this comment

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

The fact we were not able to remove PHP 8.0 entirely for 23.05 is already less than ideal: PHP 8.0 security supports ends a bit before the sunset of 23.05.

I'm still not sure to understand what's the point of marking PHP 8.0 as broken right now? It is likely to confuse users as they will end up compiling the package instead of pulling it from the binary cache and there is no easy way to understand why. PHP 8.0 can still be built and installed with no issues which is what meta.broken is about.

I wouldn't advocate for keeping it if there were no users but in this case we do have significant users within Nixpkgs. We're even using limesurvey right now for this year's community survey.

PHP 8.1 was released 1.5 year ago, if upstream cannot keep up with PHP releases resources are probably best spent on working to help them with the compat issues instead of trying to keep it in a somewhat working state in nixpkgs that will ultimately be insecure.

For Limesurvey since bc48fa8 the 6.x version was released and it supports PHP 8.1.

https://manual.limesurvey.org/LimeSurvey_roadmap#LimeSurvey_6.0
https://manual.limesurvey.org/index.php?title=Installation_-_LimeSurvey_CE&type=revision&diff=176140&oldid=175637

Copy link
Member Author

Choose a reason for hiding this comment

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

The fact we were not able to remove PHP 8.0 entirely for 23.05 is already less than ideal: PHP 8.0 security supports ends a bit before the sunset of 23.05.

Agreed.

I'm still not sure to understand what's the point of marking PHP 8.0 as broken right now?

Again, it won't mark it as broken just yet. It will be marked broken once the next release maintainer bumps the version before branch-off which, in this case, should be right around the time it goes EOL anyways.

It is likely to confuse users as they will end up compiling the package instead of pulling it from the binary cache and there is no easy way to understand why.

Agreed. Though until we have a better way to do this, this is how it's done unfortunately.

Another way to do it would be to keep it unbroken but start adding CVEs as they come in; marking the package as insecure. I don't really like this option because it'd communicate to the user that php80 is still supported and the expectation of supported software on a stable release is to stay supported, which it won't. Also maintenance burden.

An alternative that I have thought of/discovered just now would be to treat it like openssl_1_1; add a pseudo vulnerability:

knownVulnerabilities = [
"OpenSSL 1.1 is reaching its end of life on 2023/09/11 and cannot be supported through the NixOS 23.05 release cycle. https://www.openssl.org/blog/blog/2023/03/28/1.1.1-EOL/"
];

Since there's precedent for this in Nixpkgs, would you be fine with that?

PHP 8.1 was released 1.5 year ago, if upstream cannot keep up with PHP releases resources are probably best spent on working to help them with the compat issues instead

This is the best solution. I wish it was an option. Nobody should use or support EOL software.

Grocy upstream is not very cooperative and does not see any issue.

They've got 8.1 support in their dev branch but I wouldn't be comfortable shipping it. I have no idea about PHP, so I couldn't do a backport either.

For Limesurvey since bc48fa8 the 6.x version was released and it supports PHP 8.1.

Great!

@Atemu Atemu requested a review from LeSuisse June 17, 2023 13:37
@Atemu
Copy link
Member Author

Atemu commented Jun 25, 2023

PHP 8.0 has been removed :/

@Atemu Atemu closed this Jun 25, 2023
@Atemu Atemu deleted the php8.0-mark-unsupported branch June 25, 2023 15:59
@etu
Copy link
Contributor

etu commented Jun 25, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants