build-support/php: fix regression in buildComposerProject2
#475201
Conversation
nixpkgs-ci
bot
added
10.rebuild-linux: 11-100
liammcdermott
force-pushed
the
hotfix/fix-composer-flags
branch
2 times, most recently
from
1d2a80b to
1992b33
Compare
| composerNoPlugins ? finalAttrs.composerNoPlugins or true, | ||
| composerNoScripts ? finalAttrs.composerNoScripts or true, | ||
| composerNoDev ? true, | ||
| composerNoPlugins ? true, |
There was a problem hiding this comment.
Why those changes ? And why not doing the same to other parameters ?
There was a problem hiding this comment.
Why those changes ?
Those parameters need to be added to the top-level attrset, and I got an infinite recursion error without this change.
And why not doing the same to other parameters ?
The default parameter values were already a mishmash where some provided default values and others referenced finalAttrs, so I just made the minimal change necessary to fix the bug.
Moreover, I'm not 100% certain what the practical difference is between the different styles, but I did test that removing references to finalAttrs didn't break the functionality of composerNoDev, composerNoPlugins, and composerNoScripts.
Should I make the same change for composerLock, vendorHash, and composerStrictValidation?
There was a problem hiding this comment.
I'd like to ask @ShamrockLee for their PoV too on this, as I am a bit unsure.
There was a problem hiding this comment.
finalAttrs: { x ? finalAttrs.x or true }: { inherit x; } logically results in infinite recursion.
The expression before this change probably sets x as true silently due to the lack of inherit x;.
There was a problem hiding this comment.
What would you recommend to do here ?
There was a problem hiding this comment.
It just came to me that the intention of the original author (of all those { x ? finalAttrs.x or default }) is to effectively construct a let-in block with x = finalAttrs.x. I was persuaded into thinking that it would work when reviewing the change.
We need to get it fixed before it is forgotten, but the diff size and complexity would be greater than what we are changing now. We should place the finalAttrs fix in a subsequent PR.
There was a problem hiding this comment.
Sorry in advance if this question may be dumb but... To me this PR is complete, do you suggest something else to do in another PR?
There was a problem hiding this comment.
Sorry in advance if this question may be dumb but... To me this PR is complete, do you suggest something else to do in another PR?
There was a problem hiding this comment.
To me this PR is complete, do you suggest something else to do in another PR?
Yes. Sorry for making this out-of-context comments.
This PR is removing the hints of wrong implementation by cleaning up its leftovers, just bringing it up so that people going through this PR would be aware of the TBD fix.
Perhaps I should open an issue or simply create the PR.
There was a problem hiding this comment.
Yes please do it, so I can also see what you have in mind :)
liammcdermott
force-pushed
the
hotfix/fix-composer-flags
branch
from
1992b33 to
14f900d
Compare
…ey are consistent with buildComposerProject
…buildComposerProject
|
@liammcdermott Mind running I want to make sure this change doesn't break any derivation using this builder. You could try to run some commands locally to verify:
|
| [[ 1 == "${composerNoDev:-1}" ]] && composerFlags+=(--no-dev) | ||
| [[ 1 == "${composerNoPlugins:-1}" ]] && composerFlags+=(--no-plugins) | ||
| [[ 1 == "${composerNoScripts:-1}" ]] && composerFlags+=(--no-scripts) | ||
| [[ "${composerNoDev:-0}" == 1 ]] && composerFlags+=(--no-dev) |
There was a problem hiding this comment.
Nit-picking:
People typically do test -n and test -z, which is what $stdenv/setup.sh is using.
| [[ "${composerNoDev:-0}" == 1 ]] && composerFlags+=(--no-dev) | |
| [[ -n "${composerNoDev-}" ]] && composerFlags+=(--no-dev) |
(I cannot do multi-line comment on mobile GitHub pages, so please help fix other lines as well.)
|
|
The kimal build issue is related with these changes. It seems that it is no longer possible to customize the 'php' attribute. We need to find why, this cannot be merged as is. |
Copy extra.installer-paths from composerVendor before running composer install so offline builds don't fetch dependencies.
Keep composer in sync with overridden php builds so extension requirements (e.g. xsl) are satisfied.
|
The kimai issue was due to The resolution was to default Regarding commit 2ca3ef8: when testing this PR with some of our Drupal sites it was necessary to introduce a fix into @ShamrockLee thank you for the correction about The failure of firefly-iii to build was something of a corner case, since it directly invokes Sorry for the long comment. |
|
|
For me those php-codesniffer packages also fail to build on |
| vendorHash = "sha256-fLL0FAhd8r2igiZZ+wb1gse+vembHS6rzUnKe9LXXmI="; | ||
| }; | ||
|
|
||
| composerNoDev = true; |
There was a problem hiding this comment.
Why is this required? Are those attributes not true by default ?
3 participants
Fix
composerFlagsregression inbuildComposerProject2. Issue #475198Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.