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

Use native composer home #31

Merged
merged 1 commit into from
Feb 22, 2018
Merged

Use native composer home #31

merged 1 commit into from
Feb 22, 2018

Conversation

Slamdunk
Copy link
Contributor

Some services like Bitbucket use default configuration for the most common usages, for example the default cache points to the default composer cache dir:

https://confluence.atlassian.com/bitbucket/caching-dependencies-895552876.html

Altering the defaults parameters of composer with a custom $COMPOSER_HOME generates friction in the integration.

Ok, nothing that can't be solved with additional configuration, but if we can use defaults I think we should 😃

@fquffio fquffio self-assigned this Feb 22, 2018
@fquffio
Copy link
Collaborator

fquffio commented Feb 22, 2018

Totally agree this is a far better default than the one we had before.

I'm just wondering whether the COMPOSER_HOME environment variable should be removed at all, or rather set to $(composer config --global home). 🤔 Though I think this is quite unlikely, somebody may be relying on it. Maybe we could do something like this instead:

ENV COMPOSER_HOME=$(composer config --global home)
ENV PATH=$COMPOSER_HOME/vendor/bin:$PATH

@Slamdunk
Copy link
Contributor Author

We would chasing our own tail, a bit 😛
https://github.com/composer/composer/blob/ea9b7ecbb05c4b4265b1e15d72b2695439fdc12a/src/Composer/Factory.php#L56-L72

I understand if you want to retain backward compatibility, and I'll change the code if you ask me that.

Said that, if it would be up to me, I would drop it at all: the important thing is only the $PATH.

@fquffio
Copy link
Collaborator

fquffio commented Feb 22, 2018

Uhm… no, I see the problem there. It was a mistake to set that variable in the first place. 😞

As a "standard" practice, I don't think anybody should read that value from the environment directly except Composer itself, so I guess it's fine to just drop it. 👍

@fquffio fquffio merged commit 9249153 into chialab:master Feb 22, 2018
@fquffio
Copy link
Collaborator

fquffio commented Feb 22, 2018

Thanks again for your contribution!

@Slamdunk Slamdunk deleted the native_composer_home branch February 22, 2018 15:18
@Nemo64
Copy link
Contributor

Nemo64 commented Feb 23, 2018

So now there is a compatibility problem.
I already have adjusted my deployment for the composer home being in a different place.
This update will now effectively break my deployment cache.

@Slamdunk
Copy link
Contributor Author

Yes, it is an intended behaviour

@fquffio
Copy link
Collaborator

fquffio commented Feb 23, 2018

Argh… @Nemo64, sorry about that. I didn't think about that use case. Now that you point that out, it comes up to my mind that I need to update volumes paths in a couple CI pipelines as well. 😞

Is yours a cache that's expensive to be recreated?

@fquffio
Copy link
Collaborator

fquffio commented Feb 23, 2018

Would creating a symlink from /root/composer to $(composer config --global home) be a solution in your case?

@Slamdunk
Copy link
Contributor Author

Likely yes.

@fquffio I think $(...) notation isn't interpreted in Docker, a prinvent in Bitbucket says:

PATH=$(composer config --global home)/vendor/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin

We can partially revert this change to have /root/composer as a symlink, and /root/composer/vendor/bin in the path, without a new ENV VAR

@fquffio
Copy link
Collaborator

fquffio commented Feb 23, 2018

Ok, sorry again about that.

I'm in a meeting right now, so I can't address this problem in a proper way. I'm therefore temporarily reverting the merge commit, and opening an issue to restore the behaviour as of this PR.

@Nemo64
Copy link
Contributor

Nemo64 commented Feb 23, 2018

I don't think that everything needs to be compatible indefinitely.
But even with the change in place, it's the nature of docker to cache everything so when the folder is changed, some might have the updates and other may not depending on when the cache was created. The only real solution would probably be to just tag any update with something. Like 7.2-chialab3 similar how Debian packages are versioned with -debian2.

fquffio added a commit that referenced this pull request Feb 23, 2018
This reverts commit 9249153, reversing
changes made to e2020af.
@fquffio
Copy link
Collaborator

fquffio commented Feb 23, 2018

Ideally we would adhere to SemVer. This is difficult, though, because PHP is already using SemVer for its releases, and our semantic versioning would conflict with that.

Adding a suffix to the tag would do the work. We might tag each commit with -$(git rev-parse --short HEAD), so chialab/php:7.2-abcdef0, but I'd still keep the tag chialab/php:7.2 that points to the latest available image of its kind. This way one might choose between the fixed version, or the latest build.

@Nemo64 @Slamdunk What do you think?

@Slamdunk
Copy link
Contributor Author

Docker images release cycle can't be strictly compared to libary ones.

Debian style refers only to fix backporting, so it's not our case.

If you'd like to have SemVer you should use your version first: chialab/1.0/php:7.2 but it's a bit ugly.

Dunno... I would concentrate my energies in fixing (my, I'm sorry) this issue without versioning it.

Documenting all this would be good though.

@Nemo64
Copy link
Contributor

Nemo64 commented Feb 23, 2018

Right, if the issue can be resolved without breaking compatibility than go for it.

Still, I kind of dislike having mutable tags which may or may not break old projects in feature releases. Or having to tell my coworkers "do a pull before starting, there is a new extension we need". I think i'll create a separate issue for that.

@Nemo64 Nemo64 mentioned this pull request Feb 23, 2018
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.

3 participants