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

Phive > Upgrade all phar-download to phive-install #369

Merged
merged 5 commits into from
Mar 22, 2021

Conversation

mihai-stancu
Copy link
Contributor

Based on #354 I've migrated all the phar-download commands to phive-install commands.

Some didn't work tho, idk why.

Note: if the tool was installed with a specific version in the url, then I retained the version specification in the install command (alias@^version). Most tools likely didn't have a latest url setup so the version is probably unnecessary there (it's not a BC break) but I can't know that.

@mihai-stancu mihai-stancu force-pushed the phive-tools branch 3 times, most recently from d7a872e to 2f24a11 Compare March 3, 2021 07:56
@jakzal
Copy link
Owner

jakzal commented Mar 3, 2021

Note: if the tool was installed with a specific version in the url, then I retained the version specification in the install command (alias@^version). Most tools likely didn't have a latest url setup so the version is probably unnecessary there (it's not a BC break) but I can't know that.

Can we remove the version where possible?

@jakzal
Copy link
Owner

jakzal commented Mar 3, 2021

Some installations fail due to the question about key import:

Downloading https://github.com/mihaeu/dephpend/releases/download/0.7.0/dephpend-0.7.0.phar.asc

Downloading key 76835C9464877BDD
Trying to connect to keys.openpgp.org (37.218.245.50)
Downloading https://keys.openpgp.org/pks/lookup?op=get&options=mr&search=0x76835C9464877BDD

 ╰|>                                       | 0.00 KB / 3.10 KB -   0%
 ╰|========================================>| 3.10 KB / 3.10 KB - 100%
Successfully downloaded key.

Error:    Needs tty to be able to confirm
	Fingerprint: 44CC 65DC 01D2 FC05 AD6F 3DBD 7683 5C94 6487 7BDD

	Michael Haeuslmann <[email protected]>

	Created: 2019-07-14

Import this key? [y|N] 
make: *** [Makefile:33: test-integration] Error 4

@mihai-stancu
Copy link
Contributor Author

I noticed. Does that mean we should always use --trust-gpg-keys because we can't validate them beforehand?

@mihai-stancu
Copy link
Contributor Author

Note: if the tool was installed with a specific version in the url, then I retained the version specification in the install command (alias@^version). Most tools likely didn't have a latest url setup so the version is probably unnecessary there (it's not a BC break) but I can't know that.

Can we remove the version where possible?

I can remove the version constraint but IDK if there are any compatibility issued.

Do you know if phive checks the php version and restricts installs based on that in any way? For example PHPunit5 may not work with PHP8.

Point is, I'm not aware of the limitations each tool has and/or if that version specification was put there on purpose to cover a BC break or if it was there because there was no URL with "latest" available.

@mihai-stancu
Copy link
Contributor Author

Middle-ground could be alias@^<major_version_without_minor_or_build> which should at least cover PHP-level compat.

@mihai-stancu
Copy link
Contributor Author

mihai-stancu commented Mar 3, 2021

@jakzal BTW, IDK during which context phive is executed without any parameters (sometime right after install?).

The setup command should run phive --home %target-dir% and the test command should run phive --version neither of which should output the help section...

This was caused by improper use of phive --home %target_dir%.

@mihai-stancu
Copy link
Contributor Author

mihai-stancu commented Mar 3, 2021

Apparently the topic of auto-accept isn't going to be a fun one: phar-io/phive#69 ... either we echo "Y" | phive and give up on security or we create a local (manually downloaded) list of GPG keys...

@jakzal
Copy link
Owner

jakzal commented Mar 3, 2021

Is it possible to import keys beforehand in an automated way?

@mihai-stancu
Copy link
Contributor Author

mihai-stancu commented Mar 3, 2021

Is it possible to import keys beforehand?

At the begining of that thread they mentioned a "home for the GPG keys" but IDK if it's a concept that exists or if it was a proposal.

At the end of that thread they mentioned a flag you can use to specify a list of keys that are trusted (which means I was using the --trust-gpg-keys all wrong, it was supposed to be an array of key identifiers).

@jakzal phive has a directory in which it keeps a private gpg keyring. We could run the install once manually and obtain the folder that way, commit it, and have it already present when subsequent builds take place. Signature conflicts will have no automatic upgrade path (if a provider changes his sig).

Any automated import is viewed as a security risk.

@mihai-stancu mihai-stancu changed the title Phive tools Phive > Upgrade all phar-download to phive-install Mar 3, 2021
@mihai-stancu
Copy link
Contributor Author

In order to test the rest of the implementation details I've defaulted the implementation to accepting unsigned packages.

@mihai-stancu mihai-stancu force-pushed the phive-tools branch 5 times, most recently from c79d447 to c9aeae5 Compare March 4, 2021 07:23
@mihai-stancu
Copy link
Contributor Author

@jakzal the current failure is a permission denied on box but I can't see the full command line being executed, IDK where it's coming from. Can you help?

@jakzal
Copy link
Owner

jakzal commented Mar 4, 2021

It seems that box is downloaded to the box directory:

Downloading https://github.com/box-project/box/releases/download/3.11.1/box.phar
Linking /Users/kuba/Projects/toolbox/local/phars/humbug/box-3.11.1.phar to /Users/kuba/Projects/toolbox/local/box/box

By the way, you can install tools locally for easier debugging:

mkdir local
COMPOSER_HOME=$(pwd)/local/composer PATH=$(pwd)"/local:"$(pwd)"/local/composer/vendor/bin:$PATH" bin/toolbox.php install --target-dir ./local --tag pre-installation

you'll then see that box is a directory:

ls local/
_tmp_wrk/     box/          box-legacy*   composer/     gpg/          http-cache/   phars/        phive*        registry.xml

@mihai-stancu
Copy link
Contributor Author

@jakzal the target parameter of phive only accepts/expects a directory phive install -t <target_directory> as far as i can see you cannot specify the filename.

We have at least 3 instances where we artificially alter the tool name (ex.: phpunit versions).

@jakzal
Copy link
Owner

jakzal commented Mar 5, 2021

We need to think how to work around it.

What's the difference between --home and --target?

Perhaps we could install all phive tools to their own directories grouped under phive %target-dir%/phive/tool-name, and then link each one of them to %target-dir%/tool-name. Just thinking out loud as I haven't thought that through yet.

@mihai-stancu
Copy link
Contributor Author

phive has its own home directory where it keeps:

  • gpg -- it's private keyring for accepted key sigs
  • phars -- the actual phar files in the format <name>-<semver>.phar
  • some xml metadata it needs for normal operations
  • a teporary folder where it keeps downloaded stuff before moving them

By default when you install something it will symlink it into the current folder under the short (versionless) name.
By default when you global install something it will symlink it into /usr/local/bin with the short versionless name.

If you specify -t <target> you're specifying the directory in which to place the symlink.

We can work around it by allowing it to generate a symlink somewhere else (in a "clean" directory such as /tools/.phive/some_clean_tmp_dir) and moving it from there to our desired location. Feels kludgy but it'll do the job.

@mihai-stancu mihai-stancu force-pushed the phive-tools branch 2 times, most recently from 853860f to c7c43a5 Compare March 8, 2021 23:38
@mihai-stancu
Copy link
Contributor Author

This just dawned on me: each command.phive-install.command could have a property signature to execute phive install --trust-gpg-signature $signature`.

IDK how I missed this, this was very obvious.

@mihai-stancu mihai-stancu force-pushed the phive-tools branch 4 times, most recently from 4515df8 to d75ef54 Compare March 18, 2021 08:13
@jakzal
Copy link
Owner

jakzal commented Mar 18, 2021

Starts looking good 👍

Builds are currently failing due to an issue unrelated to this PR: deprecated-packages/symplify#3056

resources/tools.json Outdated Show resolved Hide resolved
@mihai-stancu mihai-stancu force-pushed the phive-tools branch 2 times, most recently from 671ae21 to d0dec93 Compare March 18, 2021 23:46
@mihai-stancu
Copy link
Contributor Author

Did you disable ecs?

That allowed the tests to continue up until they hit phpbench which wasn't working for PHP8.

Now everything works. Yay!

@jakzal
Copy link
Owner

jakzal commented Mar 19, 2021

Did you disable ecs?

No, they seem to have fixed the phar :)

@jakzal
Copy link
Owner

jakzal commented Mar 19, 2021

@mihai-stancu I will make a new toolbox and phpqa releases. This will give you some time to finalise this PR.

Once ready, we'll merge it and release toolbox.

@mihai-stancu
Copy link
Contributor Author

mihai-stancu commented Mar 19, 2021

Status:

  • GPG keys are now handled nicely via key signatures + keyservers
  • phpqa image has been updated with gpg packages, that build is currently failing due to phpstan
  • ugly '/tmp/'.md5($alias) has not been addressed -- do you have any ideas?

@jakzal
Copy link
Owner

jakzal commented Mar 22, 2021

@mihai-stancu can you rebase again please?

@jakzal
Copy link
Owner

jakzal commented Mar 22, 2021

Also, seems we can't be using psalm as phar to use psalm plugins (see #252). Can you revert it back please?

@jakzal
Copy link
Owner

jakzal commented Mar 22, 2021

I pushed 994f20a to your branch. This is an idea to work around the ugly /tmp usage. It also makes that phive files and directories are not polluting the /tools/ directory but are nicely contained in /tools/.phive. WDYT?

As a result, we don't use the "bin part" of the bin option:

"command": {
    "phive-install": {
        "alias": "dephpend",
        "bin": "%target-dir%/dephpend",
        "sig": "76835C9464877BDD"
    }
}

How about we turn it into a target (or whatever phive is calling it)?

"command": {
    "phive-install": {
        "alias": "dephpend",
        "target": "%target-dir%",
        "sig": "76835C9464877BDD"
    }
}

@mihai-stancu
Copy link
Contributor Author

Rebased. Checks passed.

@jakzal
Copy link
Owner

jakzal commented Mar 22, 2021

Outstanding work! Thank you @mihai-stancu 🍻

@jakzal jakzal merged commit 4cc18c3 into jakzal:master Mar 22, 2021
@mihai-stancu
Copy link
Contributor Author

\m/ (-_-) \m/

🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants