Conversation
4f6716d to
a47c9cb
Compare
a47c9cb to
3245af0
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
@Al2Klimov would you test it. There are still some failures as we need to merge other PRs for extension upgrades, but core should work. |
2ebd1b6 to
a27272c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
eaeeb68 to
9a46efb
Compare
|
Ma27
left a comment
There was a problem hiding this comment.
Thanks!
Even though I'm still not particularly happy about this de-facto vendoring of lexbor, just including the headers where necessary is IMHO the way better solution.
No final approval yet because CI is red.
@NixOS/nixpkgs-ci is there a way to reasonably add versioned attributes into by-name now given that this is what the CI wants from us?
Without looking at the specifics of this PR, yes this is possible. One approach is to create a by-name package directory for each version. Each version-package then overrides the base package (since it can't reference paths outside its scope). Alternatively, you can expose versioned-packages as passthru attrs. Each version-package can reference |
Well, this is a new versioned attribute and not used anywhere yet. Also, I disagree with this being acceptable. By this rationale we could kill most Hydra jobs because the packages happen to be built in e.g. VM tests. This isn't helpful at all though when having dedicated jobsets to track regressions after core updates of e.g. glibc. I honestly think this should be used in exceptional cases at best.
To me, this still feels like a hack to force-push versioned attributes into something that doesn't really fit, by-name. |
|
Perhaps we could acknowledge that we should improve this and have another dedicated PR for it? So we don't block this long and awaited PR ? |
|
The reason I'm bringing this up is that the CI is red and IIRC "no PR failures" means we get kicked out of the queue when trying to merge this. EDIT: to be clear, the approaches outlined by @MattSturgeon are the workarounds I've seen for a while now on trying to push ~everything into by-name. I'd feel very uneasy to do this for versioned attributes like we have here for the reasons I explained above. |
|
Alternately, we could merge without waiting for requirements and deal with this later. |
|
Yeah, it's OK to let the ratchet go back a notch. |
| --replace-fail "$out/lib" "$dev/lib" | ||
| '' | ||
| + lib.optionalString (lib.versionAtLeast version "8.5") '' | ||
| # PHP 8.5+ has lexbor built into core; dom needs its headers. |
There was a problem hiding this comment.
Note for later: remove the comment from the script, move it outside.
Add a release note.
|
Successfully created backport PR for |
Introduced in NixOS/nixpkgs#481104
Introduced in NixOS/nixpkgs#481104
Supersedes #422308
Notes:
opcache is now built-in. I run a test using
var_dump(function_exists('opcache_get_status'));uri extension works. Tested using:
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.