search

eudev-project / eudev

Repository for eudev development
GNU General Public License v2.0
525 stars 144 forks source link

Permit eudev to work with rules which include escaped double-quotes #208

Closed slicer69 closed 3 years ago

slicer69 commented 3 years ago

This commit resolves GitHub issue #187.

slicer69 commented 3 years ago

Hi all. I see that a couple of CI checks are failing here, presumably during the "make check" run. I ran this on my own system before submitting the pull request and I'm getting 2/2 checks passed locally, but it looks like the new udev-test.pl script is failing on the CI container.

Anyone else else willing to try out my above patch and see if the "make check" test passes for them? I'm curious if the error is distro-specific (I'm on Debian rather than Devuan or Alpine where the tests are failing.)

bbonev commented 3 years ago

Don't worry about the CI - one of the tests in eudev fails every time when run in containers.

That is commit 7e760b79ad143b26a5c937afa7666a7c40508f85; maybe it is a good idea to have the original commit info + message in our commit message for easier reference, also credit to the original authors (it is not yet decided what is the best way to do that)

slicer69 commented 3 years ago

Don't worry about the CI - one of the tests in eudev fails every time when run in containers.

Thanks for letting me know. I was scratching my head on that one for a few minutes.

That is commit 7e760b79ad143b26a5c937afa7666a7c40508f85; maybe it is a good idea to have the original commit info + message in our commit message for easier reference, also credit to the original authors (it is not yet decided what is the best way to do that)

Good point. I've updated the commit message to credit the upstream patch author and added their commit message.

bbonev commented 3 years ago

I need some time to review.

In the meantime, isn't it a good idea to force push only the last change, or do you think it's better to squash merge? All the history accumulated is pointless in the main repo, IMO

slicer69 commented 3 years ago

In the meantime, isn't it a good idea to force push only the last change, or do you think it's better to squash merge? All the history accumulated is pointless in the main repo, IMO

I don't feel strongly about it either way, so I'm happy to follow your lead. If you feel force pushing the lat change is the way to go, then I'm on board with it.