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

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

Closed
wants to merge 4 commits into from

Conversation

drupol
Copy link
Contributor

@drupol drupol commented May 2, 2024

Some projects in PHP are running composer commands on their CI before building a release.

Even if the project's composer.lock gets shipped along the PHAR release, running composer validate returns and error (bobthecow/psysh#796).

This PR allows users to run a preBuild hook so that extra composer commands can be triggered just before building the composer repository (017b29d)

The downside of this is that all the PHP hashes needs to be regenerated. I don't know if there's a better way to do this yet.

Description of changes

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 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.

Add a 👍 reaction to pull requests you find important.

@ofborg ofborg bot requested review from Ma27, aanderse and talyz May 2, 2024 20:50
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 labels May 2, 2024
@drupol drupol force-pushed the psysh/bump/0-12-3 branch from 0cfc3e3 to 47bfa0e Compare May 2, 2024 20:57
@@ -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.

@@ -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.

@@ -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.

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?

vendorHash = "";

composerRepository = {
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.

@@ -16,7 +16,18 @@ php.buildComposerProject (finalAttrs: {
url = "https://github.com/bobthecow/psysh/releases/download/v${finalAttrs.version}/composer-v${finalAttrs.version}.lock";
hash = "sha256-ur6mzla3uXeFL6aEHAPdpxGdvcgzOgTLW/CKPbNqeCg=";
};
vendorHash = "";

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.

@drupol
Copy link
Contributor Author

drupol commented May 5, 2024

superseded by #308059

@drupol drupol closed this May 5, 2024
@drupol drupol deleted the psysh/bump/0-12-3 branch May 5, 2024 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants