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.

Sign up for GitHub

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

psysh: 0.11.21 -> 0.12.3 + build-support/php: update #308608

Closed
wants to merge 4 commits into from
Closed

psysh: 0.11.21 -> 0.12.3 + build-support/php: update #308608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7f75d07
php.packages.psysh: 0.11.21 -> 0.12.3
drupol May 2, 2024
880bbde
build-support/php: let the `mkComposerRepository` be overridable
drupol May 2, 2024
7a3eb2e
build-support/php: ship `composer.json` in composer repository
drupol May 2, 2024
47bfa0e
psysh: fix 0.12.3 using a `preBuild` hook
drupol May 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pkgs/build-support/php/build-composer-project.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ let
runHook postInstallCheck
'';

composerRepository = phpDrv.mkComposerRepository {
composerRepository = phpDrv.mkComposerRepository ({
Copy link
Member

Choose a reason for hiding this comment

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

I would use previousAttrs.composerRepository or phpDrv.mkComposerRepository …. This looks more like composerRepositoryArgs and having different behaviour when passing it to buildComposerProject and to overrideAttrs would be confusing.

inherit composer composer-local-repo-plugin;
inherit (finalAttrs) patches pname src vendorHash version;

Expand All @@ -71,7 +71,7 @@ let
composerNoPlugins = previousAttrs.composerNoPlugins or true;
composerNoScripts = previousAttrs.composerNoScripts or true;
composerStrictValidation = previousAttrs.composerStrictValidation or true;
};
} // (previousAttrs.composerRepository or {}));

COMPOSER_CACHE_DIR="/dev/null";
COMPOSER_DISABLE_NETWORK="1";
Expand Down
6 changes: 4 additions & 2 deletions pkgs/build-support/php/hooks/composer-install-hook.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ composerInstallConfigureHook() {
cp "$composerLock" composer.lock
fi

chmod +w composer.{json,lock}
cp ${composerRepository}/composer.{json,lock} .
Copy link
Member

Choose a reason for hiding this comment

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

This makes the two lock checks below redundant.

chmod +w composer.{json,lock}

if [[ ! -f "composer.lock" ]]; then
setComposeRootVersion

Expand Down Expand Up @@ -71,8 +75,6 @@ composerInstallConfigureHook() {
exit 1
fi

chmod +w composer.json composer.lock

echo "Finished composerInstallConfigureHook"
}

Expand Down
4 changes: 3 additions & 1 deletion pkgs/build-support/php/hooks/composer-repository-hook.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ composerRepositoryConfigureHook() {
cp $composerLock composer.lock
fi

chmod +w composer.{json,lock}

if [[ ! -f "composer.lock" ]]; then
setComposeRootVersion

Expand Down Expand Up @@ -85,7 +87,7 @@ composerRepositoryInstallHook() {

# Copy the composer.lock files to the output directory, to be able to validate consistency with
# the src composer.lock file where this fixed-output derivation is used
cp composer.lock $out/
cp composer.{json,lock} $out/

echo "Finished composerRepositoryInstallHook"
}
2 changes: 1 addition & 1 deletion pkgs/development/php-packages/composer/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ php.buildComposerProject (finalAttrs: {
--prefix PATH : ${lib.makeBinPath [ _7zz cacert curl git unzip xz ]}
'';

vendorHash = "sha256-dNNV9fTyGyRoGeDV/vBjn0aMgkaUMsrKQv5AOoiYokQ=";
vendorHash = "sha256-4s912b04PTPbK9+suni9uJ2Cx4qDb8CkMLy6y25a1eQ=";
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is bad. If we want to not break BC for outside consumers, we would need to introduce mkComposerRepository2 and buildComposerProject2, and deprecate the old one.


meta = {
changelog = "https://github.com/composer/composer/releases/tag/${finalAttrs.version}";
Expand Down
25 changes: 20 additions & 5 deletions pkgs/development/php-packages/psysh/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,33 @@
{ fetchFromGitHub, lib, php }:
{ fetchFromGitHub, fetchurl, lib, php }:

php.buildComposerProject (finalAttrs: {
pname = "psysh";
version = "0.11.21";
version = "0.12.3";

src = fetchFromGitHub {
owner = "bobthecow";
repo = "psysh";
rev = "v${finalAttrs.version}";
hash = "sha256-YuBn4mrgOzGeMGfGcyZySAISmQdv3WRGn91PRozyxdI=";
hash = "sha256-v2UAhxnfnVwA05sxcqMU5vmQcwlBcc901PYJqYf+pCw=";
};

composerLock = ./composer.lock;
vendorHash = "sha256-FZFeO7UiVssxTf0JX6wdjrAE+jucYnfQJA1eOng39lQ=";
composerLock = fetchurl {
name = "composer.lock";
url = "https://github.com/bobthecow/psysh/releases/download/v${finalAttrs.version}/composer-v${finalAttrs.version}.lock";
hash = "sha256-ur6mzla3uXeFL6aEHAPdpxGdvcgzOgTLW/CKPbNqeCg=";
};

composerRepository = {
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is confusing. I would expect this attribute to be passed the result of mkcomposerRepository.

If you apply the suggested change to buildComposerProjectOverride, you should be able to use composerRepository = previousAttrs.composerRepository.overrideAttrs { preBuild = …; };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'm not yet at ease with this.... I will do the change.

preBuild = ''
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, maybe we should use make build/psysh then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

make ? What does make has anything to do in here?

Copy link
Member

Choose a reason for hiding this comment

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

You are mentioning it in the commit message.

setComposeRootVersion
composer config platform.php 7.4
composer require --no-update symfony/polyfill-iconv symfony/polyfill-mbstring
composer require --no-update --dev roave/security-advisories:dev-latest
Copy link
Member

Choose a reason for hiding this comment

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

Actually, this is not reproducible, is it? The hash would change over time since the lockfile is included in the output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is fully reproducible actually, tested with --rebuild.

Copy link
Member

Choose a reason for hiding this comment

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

What if roave/security-advisories releases a new version?

composer update --no-interaction --no-progress --prefer-stable --no-dev --classmap-authoritative --prefer-dist --lock
'';
};

vendorHash = "sha256-vlEbehxy6xi2qLKG32fV0OJVSphWjqKUVHbWOhoWjoI=";

meta = {
changelog = "https://github.com/bobthecow/psysh/releases/tag/v${finalAttrs.version}";
Expand Down