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

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

Merged
merged 1 commit into from
Oct 10, 2021

Conversation

slicer69
Copy link

@slicer69 slicer69 commented Oct 5, 2021

  • Adjust parsing function to understand escaped double-quotes in rules.
  • Added new test to udev-test.pl to confirm escaped quotes work.
  • Bring eudev up to date with systemd pull request $6890.

This commit resolves GitHub issue #187.

@slicer69
Copy link
Author

slicer69 commented Oct 5, 2021

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
Copy link
Member

bbonev commented Oct 6, 2021

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
Copy link
Author

slicer69 commented Oct 6, 2021

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
Copy link
Member

bbonev commented Oct 6, 2021

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
Copy link
Author

slicer69 commented Oct 6, 2021

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.

- Adjust parsing function to understand escaped double-quotes in rules.
- Added new test to udev-test.pl to confirm escaped quotes work.
- Bring eudev up to date with systemd pull request $6890.

This commit resolves GitHub issue eudev-project#187.

This is a slightly adjusted patch/commit from systemd's udev.
Original upstream commit: 8a247d50abb5315adcc64daea10bc836c9d4c6e0
Provided by Franck Bui (GitHub user fbuihuu)
Copy link
Member

@bbonev bbonev left a comment

Choose a reason for hiding this comment

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

LGTM

@bbonev bbonev merged commit ca859c8 into eudev-project:master Oct 10, 2021
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