composer-patches icon indicating copy to clipboard operation
composer-patches copied to clipboard

Fix a Unix breaking bug caused by a hack

Open Techwolf12 opened this issue 7 years ago • 39 comments

~~This first checks if you are running windows before changing the patch command to a flag not supported on FreeBSD.~~ Patch is given the --posix flag now.

Should be a fix for #182 If someone with access to multiple windows machines can test this a bit more thoroughly, would be appreciated!

Techwolf12 avatar Dec 19 '17 18:12 Techwolf12

This is not just a Windows thing. --no-backup-if-mismatch exists in whatever version of patch comes with Ubuntu as well.

FreeBSD is not high on my priority list, mostly because it's one of those things that will constantly break. To my knowledge, there aren't any services similar to Travis CI that provide a way to do automated testing on FreeBSD, so I cannot make any guarantees about it.

If you can find a service that offers FreeBSD, I'm not opposed to looking into official FreeBSD support here, but until that point, my inclination is to recommend that you use a more standard setup.

cweagans avatar Jan 15 '18 23:01 cweagans

The reason the flag is used is that you want POSIX behaviour on Patch, in that case, you can also use "--posix" as supported on Linux (GNU Patch) & POSIX Patch.

This is why I changed the flag to --posix. Please merge it? Official support cannot be expected but I fixed this issue while being fully backwards compatible. Why not merge it in?

Techwolf12 avatar Jan 16 '18 08:01 Techwolf12

We are not asking for official FreeBSD support, I completely understand that would take considerably more time. The above patch however keeps backwards-compatibility, and will instantly fix 4 outstanding issues, so I see no reason to not accept this request.

martijnhoutman avatar Jan 16 '18 11:01 martijnhoutman

I can get behind using the --posix flag. It's a superset of what --no-backup-if-mismatch does, but that's okay.

Note also that master is where the future 2.0.0 release will live. That release is going to take some time to happen, so you might consider opening a similar PR against 1.x.

cweagans avatar Jan 16 '18 17:01 cweagans

There's something funky going on with Travis today. I'm not gonna merge this until the tests pass, but I think the tests shouldn't be affected by this change. I'll investigate the Travis issues later and then this should be good to go.

cweagans avatar Jan 16 '18 18:01 cweagans

only failures showing atm are "allowed" so can has fix? ;)

neclimdul avatar Jan 23 '18 19:01 neclimdul

Can confirm that 33fb8ed573413e09f10efef011046fac5eed10e1 is working for me on Alpine Linux 3.6/3.7

joergLin avatar Jan 24 '18 07:01 joergLin

@cweagans Can you try rebuilding in travis? Hopefully it works. I also created #190 for the 1.x branch

Techwolf12 avatar Jan 25 '18 12:01 Techwolf12

@Techwolf12 I've tried rebuilding a couple times over the past few days, and composer just stalls on the php 7.1 acceptance tests. I'm not really sure what's going on there, but I'm also not really in a position to spend a bunch of time debugging it. Can you close this PR and reopen off of the same branch? Maybe there's some stuck cache on Travis or something.

cweagans avatar Jan 25 '18 17:01 cweagans

It seems to be composer cache, maybe there is a flag to build composer without cache?

Techwolf12 avatar Jan 26 '18 10:01 Techwolf12

I cleared the caches in the Travis UI. Not sure what's going on. I'll try to look into it this weekend.

cweagans avatar Jan 26 '18 18:01 cweagans

As already flagged on #159 this fix won't address a similar issue with BusyBox, which runs on most docker images including the composer official one. Neither --no-backup-if-mismatch, nor --posix, nor -d switches exist in there.

gambry avatar Feb 19 '18 21:02 gambry

In the meanwhile to fix the issue mentioned above with docker composer official image I've create a POC image gambry/composer. I'm going to test it a bit and if fixes the problem for running composer/patches through docker composer I will create a proper reliable docker repository, with test coverage.

(Sorry for the cross-post. I think there may be people like me using the docker composer official image and crying due the issue.)

gambry avatar Feb 24 '18 11:02 gambry

@cweagans Travis tests passed, can we merge this now ?

Thanks!

drupol avatar Mar 24 '18 02:03 drupol

Please... this is getting annoying

markhalliwell avatar Mar 31 '18 07:03 markhalliwell

I'm running a fork on our company Gitlab instance to get around this currently

Techwolf12 avatar Apr 01 '18 00:04 Techwolf12

Yeah, would be really nice to have this fix. We've got workarounds in a lot of our internal build processes as well.

@Techwolf12 probably wouldn't hurt to flatten your commits.

neclimdul avatar Apr 02 '18 14:04 neclimdul

I wrote an email to Cameron, I hope we'll get an answer very soon.

drupol avatar Apr 03 '18 12:04 drupol

Hi all,

I got a reply from @cweagans , he's going to take care of this next week and Drupalcon !

Thanks in advance!

drupol avatar Apr 03 '18 14:04 drupol

drupol have you heard anything more about this getting resolved?

criley avatar Apr 12 '18 15:04 criley

Yes, I wrote the maintainer and he's going to work on this during the Drupalcon, which is happening now...dunno nothing else :(

drupol avatar Apr 12 '18 16:04 drupol

We changed the flag to -f because --posix had some weird behaviour when files didn't exist yet (/dev/null as input file)

Feel free to (temporarily) use the following, please do note that this will be an unmaintained repo and can be pulled offline at any time by us. You should use the official repo when it is fixed. It also contains my fix for the Lightning profile (which uses a different document root and breaks core patching): #204

Edit the repositories in your composer.json:

"repositories": {
        "drupal": {
            "type": "composer",
            "url": "https://packages.drupal.org/8"
        },
        "cweagans/composer-patches": {
            "type": "vcs",
            "url": "https://gitlab.uncinc.nl/pub/composer-patches"
        }
    },

Then run: composer require "cweagans/composer-patches":"dev-bsd as 1.x-dev"

(as 1.x-dev fixes dependencies on composer-patches)

Techwolf12 avatar Apr 12 '18 16:04 Techwolf12

Thanks Techwolf but I will wait for the official version to get fixed although that is not making the boss happy.

criley avatar Apr 16 '18 16:04 criley

@criley If you want, you can clone/fork my repo: https://gitlab.uncinc.nl/pub/composer-patches and run it internally on your own git server?

Techwolf12 avatar Apr 16 '18 17:04 Techwolf12

13f91edc59c conflict should be easy to fix. just a file move.

neclimdul avatar Apr 16 '18 21:04 neclimdul

Hello. I still can't use the package on FreeBSD due to the --no-backup-if-mismatch flag. Is there any chance for this to be fixed soon? Also, tell me if I can help in any way to get this fixed.

babisp avatar Jul 19 '18 10:07 babisp

If I understand the github UI correctly, it looks like this branch no longer applies. Can you rebase this? So that when @cweagans gets back to this issue, there won't be any failures on this.

joris-vercammen-cali avatar Nov 12 '18 15:11 joris-vercammen-cali

@joris-vercammen-cali I'll be happy to do it if it would be merged, which I don't think will happen. See https://github.com/cweagans/composer-patches/pull/184#issuecomment-380871460 for a hacky 'fix' if you want to use it in your project.

Techwolf12 avatar Nov 12 '18 18:11 Techwolf12

I left some feedback above that was not taken into account. I assumed that you had lost interest, so I haven't prioritized this.

  1. Needs a rebase
  2. Needs Travis to finish properly
  3. Probably needs a companion PR against the 1.x branch if you want to use this anytime soon.

I'm happy to merge this when at least 1 and 2 are done.

cweagans avatar Nov 13 '18 18:11 cweagans

@Techwolf12 can fix do the issues that @cweagans mentioned?

joris-vercammen-cali avatar Nov 22 '18 11:11 joris-vercammen-cali

In case of alpine container following steps helped me. Patch version (2.7.6) have an option --no-backup-if-mismatch.

echo '@em http://dl-cdn.alpinelinux.org/alpine/edge/main' >> /etc/apk/repositories
apk add patch@em

dbjpanda avatar Feb 01 '19 12:02 dbjpanda

In case of alpine container following steps helped me. Patch version (2.7.6) have an option --no-backup-if-mismatch.

echo '@em http://dl-cdn.alpinelinux.org/alpine/edge/main' >> /etc/apk/repositories
apk add patch@em

Patch 2.7.6 seems to have landed in Alpine Linux v3.9 main repository. So the above hack may not be necessary anymore.

mareksuscak avatar Apr 24 '19 00:04 mareksuscak

The acceptance tests hang forever because of a prompt. If line 27 of c45ce03358 is changed to the following, then all tests succeed:

"patch %s --posix --batch -d %s < %s"

zrhoffman avatar Apr 24 '19 02:04 zrhoffman

I had the same issue running on Docker. I decided to fix by upgrading patch. This is my docker file

FROM drupaldocker/php:7.2-fpm-2.x

RUN apk update
RUN apk add --no-cache  patch

marcelovani avatar May 03 '19 08:05 marcelovani

In case of alpine container following steps helped me. Patch version (2.7.6) have an option --no-backup-if-mismatch.

echo '@em http://dl-cdn.alpinelinux.org/alpine/edge/main' >> /etc/apk/repositories
apk add patch@em

Patch 2.7.6 seems to have landed in Alpine Linux v3.9 main repository. So the above hack may not be necessary anymore.

On 3.10 is still affected, for whatever reason, I had to explicitly install patch package to get the unaffected newest version, then it worked.

lukasz-zaroda avatar Jul 24 '19 10:07 lukasz-zaroda

@lukasz-zaroda Not according to this. Did you try running apk --update add patch?

Either way I have switched over to https://github.com/vaimo/composer-patches is a little more flexible in that it offers console commands that you can use to reapply patches. It might be subject to the same issue tho. Although I think it falls back to using git apply

mareksuscak avatar Jul 24 '19 10:07 mareksuscak

So the above hack may not be necessary anymore.

FreeBSD is also still affected.

However, FreeBSD patch still supports the --posix option.

kentr avatar Feb 20 '20 21:02 kentr

@cweagans What do you think about #334 ?

pesc avatar Dec 28 '20 06:12 pesc

@kentr Should be fixed now, #334 got merged!

pesc avatar Aug 28 '21 20:08 pesc