Skip to content

Replaced the existing PHAR building implementation with Humbug/Box#1082

Merged
morozov merged 1 commit into
phpbrew:masterfrom
morozov:box
Dec 14, 2019
Merged

Replaced the existing PHAR building implementation with Humbug/Box#1082
morozov merged 1 commit into
phpbrew:masterfrom
morozov:box

Conversation

@morozov

@morozov morozov commented Dec 9, 2019

Copy link
Copy Markdown
Contributor
  1. Use bin/phpbrew as the PHAR stub to unify the behavior of the packaged and development versions.
  2. Removed no longer relevant build scripts, files, directories and their references in .git(attributes,ignore) files.
  3. Removed CONTRIBUTORS.md as outdated. Use https://github.com/phpbrew/phpbrew/graphs/contributors instead.
  4. Removed HACKING.md and todo.md as outdated.
  5. Removed .hhconfig as no longer effectively supported.

Fixes #1067.

1. Use bin/phpbrew as the PHAR stub to unify the behavior of the packaged and development versions.
2. Removed no longer relevant build scripts, files, directories and their references in .git(attributes,ignore) files.
3. Removed CONTRIBUTORS.md as outdated. Use https://github.com/phpbrew/phpbrew/graphs/contributors instead.
4. Removed HACKING.md and todo.md as outdated.
5. Removed .hhconfig as no longer effectively supported.
@morozov

morozov commented Dec 9, 2019

Copy link
Copy Markdown
Contributor Author

@theofidry, as a maintainer of Box, could you take a look and see if I got it right? Later, once it's done, I want to remove the built PHAR and the signature from the repository and move them to GitHub releases and rework the SelfUpdate command.

This way, the repository will contain only the source code.

@theofidry

theofidry commented Dec 9, 2019 via email

Copy link
Copy Markdown
Contributor

@morozov morozov added this to the Release 1.25.0 milestone Dec 14, 2019
@morozov morozov self-assigned this Dec 14, 2019
@morozov morozov merged commit 9f84add into phpbrew:master Dec 14, 2019
@morozov morozov deleted the box branch December 14, 2019 17:12
Comment thread Makefile
TEST = phpunit

.PHONY: build phar
$(TARGET): vendor $(shell find bin/ shell/ src/ -type f) box.json.dist .git/HEAD

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is the shell find necessary here? Wouldn't it be enough to have bin shell src?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why is .git/HEAD here?

@morozov morozov Dec 14, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I actually copy/pasted this from the Infection's makefile.

is the shell find necessary here? Wouldn't it be enough to have bin shell src?

In my understanding depending on the directories would not track changes in the underlying files since Make uses the mtime of the targets. Here, we don't care about the directories themselves but care about every single file inside them.

why is .git/HEAD here?

It may be not relevant right now but would be relevant if something like Ocramius/PackageVersions was used. It would consider the current branch/tag when generating the lock file. It could be indeed omitted for now.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👌

Comment thread box.json.dist
"KevinGH\\Box\\Compactor\\Json",
"KevinGH\\Box\\Compactor\\Php"
],
"check-requirements": false,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess the requirement checker is causing problems here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, I just excluded it to make the result closer to what it used to be previously. When we bump the PHP requirements, I'll consider enabling it back.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No, I just excluded it to make the result closer to what it used to be previously.

Makes total sense

When we bump the PHP requirements, I'll consider enabling it back.

IMO it's more about when you want to give it a try. Even if you don't bump the PHP requirements, it also checks extensions provided they are properly listed in the composer.json

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is very true. It was just out of scope. I'm totally open to enabling it and making sure all required extensions are listed in composer.json.

@theofidry

Copy link
Copy Markdown
Contributor

Looks good to me. I have a couple of questions but it's more out of curiosity than anything suspicious/wrong

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error notice when run ./scripts/compile in php-7.4.0

2 participants