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

Fix for #18 - support running with --no-scripts flag #33

Merged
merged 6 commits into from
Jul 22, 2016

Conversation

Ocramius
Copy link
Owner

Provides fix for #18

@Ocramius Ocramius added the bug label Jul 22, 2016
@Ocramius Ocramius added this to the 1.1.0 milestone Jul 22, 2016
@Ocramius Ocramius self-assigned this Jul 22, 2016
private static function getComposerLockPath() : string
{
// bold assumption, but there's not here to fix everyone's problems.
$checkedPaths = [__DIR__ . '/../../../../../composer.lock', __DIR__ . '/../../composer.lock'];

Choose a reason for hiding this comment

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

@Ocramius Perhaps some $path = dirname(__DIR__, $level++) . '/composer.lock'-based check as a better chance alternative? Of course with some reasonable threshold for $level increment.

Copy link
Owner Author

Choose a reason for hiding this comment

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

@nikolaposa I don't trust intermediate levels

@nikolaposa
Copy link

And what about some totally different approach of triggering Installer at certain event (autoloading?), or at least lazy loading (replacing) this dynamic Versions class after getVersion() is invoked for the first time?

@Ocramius
Copy link
Owner Author

And what about some totally different approach of triggering Installer at certain event (autoloading?)

Assuming read-only FS for vendor after deployment. Otherwise, we introduce possible security issues. I simply want to allow usage for people running with --no-scripts, but I don't really care if they have a performance impact then: their problem for not optimizing at install-time.

@Ocramius
Copy link
Owner Author

Alright, this goes in and I will make a release :-)

@Ocramius Ocramius merged commit fe7e599 into master Jul 22, 2016
@Ocramius Ocramius deleted the fix/#18-support-running-with--no-scripts-flag branch July 22, 2016 14:38
@stof
Copy link
Contributor

stof commented Jul 22, 2016

There is a big drawback in this PR: it means that a normal install will leave the package in a dirty state, which will cause issues on next updates for people installing from source

@Ocramius
Copy link
Owner Author

@stof yeh, no real way around it, for now :-\

);
}

rename(__DIR__ . '/../../composer.lock.backup', __DIR__ . '/../../composer.lock');
Copy link
Contributor

Choose a reason for hiding this comment

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

this will not rename things properly when there is a test failure

Copy link
Contributor

Choose a reason for hiding this comment

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

this should be done in a try/finally

Copy link
Owner Author

Choose a reason for hiding this comment

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


private static function getVersions(string $composerLockFile) : \Generator
{
$lockData = json_decode(file_get_contents($composerLockFile), true);
Copy link
Contributor

Choose a reason for hiding this comment

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

you should actually check installed packages, not locked ones. packages-dev will always be there in Composer 1.0+, even dev deps are not installed

Copy link
Owner Author

Choose a reason for hiding this comment

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

Is there an array key with the installed packages? I currently merge packages-dev + packages, and I know that dev packages may not have been installed.

Copy link
Contributor

Choose a reason for hiding this comment

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

installed packages are not in the lock file. they are in vendor/composer/installed.json (and btw, the path between your package and this folder will always be the same even when using custom vendor directories, as they are both inside it)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Right, but what interests us is just the version of a package. For example,
I may have generated(in a CI/CD environment running with dev dependencies)
an asset management system that generates files. Even though the package
doesn't make it to production, the version of the asset manager used to
generate the files is relevant for signature verification purposes. That is
the primary purpose of this library, at least in how I envisioned it when I
built it for my own needs.

On 22 Jul 2016 17:17, "Christophe Coevoet" [email protected] wrote:

In src/PackageVersions/FallbackVersions.php
#33 (comment)
:

  •        if (file_exists($path)) {
    
  •            return $path;
    
  •        }
    
  •    }
    
  •    throw new \UnexpectedValueException(sprintf(
    
  •        'PackageVersions could not locate your `composer.lock` location. This is assumed to be in %s. '
    
  •        . 'If you customized your composer vendor directory and ran composer installation with --no-scripts, '
    
  •        . 'then you are on your own, and we can\'t really help you. Fix your shit and cut the tooling some slack.',
    
  •        json_encode($checkedPaths)
    
  •    ));
    
  • }
  • private static function getVersions(string $composerLockFile) : \Generator
  • {
  •    $lockData = json_decode(file_get_contents($composerLockFile), true);
    

installed packages are not in the lock file. they are in
vendor/composer/installed.json (and btw, the path between your package
and this folder will always be the same even when using custom vendor
directories, as they are both inside it)


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
https://github.com/Ocramius/PackageVersions/pull/33/files/0a1a91ea4d08baa225724845431bee542773fd1a#r71895649,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAJakC16NEtnqktbklFgVXJ032yDe_Jcks5qYN8SgaJpZM4JSv1j
.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, so it is OK if the package is not there anymore ? This should be clearly documented then

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, if the package is not installed (because of non-dev install) then the
version will still be retrievable

On 22 Jul 2016 18:12, "Christophe Coevoet" [email protected] wrote:

In src/PackageVersions/FallbackVersions.php
#33 (comment)
:

  •        if (file_exists($path)) {
    
  •            return $path;
    
  •        }
    
  •    }
    
  •    throw new \UnexpectedValueException(sprintf(
    
  •        'PackageVersions could not locate your `composer.lock` location. This is assumed to be in %s. '
    
  •        . 'If you customized your composer vendor directory and ran composer installation with --no-scripts, '
    
  •        . 'then you are on your own, and we can\'t really help you. Fix your shit and cut the tooling some slack.',
    
  •        json_encode($checkedPaths)
    
  •    ));
    
  • }
  • private static function getVersions(string $composerLockFile) : \Generator
  • {
  •    $lockData = json_decode(file_get_contents($composerLockFile), true);
    

Oh, so it is OK if the package is not there anymore ? This should be
clearly documented then


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
https://github.com/Ocramius/PackageVersions/pull/33/files/0a1a91ea4d08baa225724845431bee542773fd1a#r71904545,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAJakNm1YtoJ-XgYCngwzUo3k-RhKxvcks5qYOv7gaJpZM4JSv1j
.

Copy link
Contributor

Choose a reason for hiding this comment

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

The readme should explain it then

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.

6 participants