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

Plugins: semlock: remove it #2411

Merged
merged 5 commits into from May 5, 2019

Conversation

KurtMi
Copy link
Contributor

@KurtMi KurtMi commented Feb 12, 2019

Basics

Do not describe the purpose here but:

  • Short descriptions should be in the release notes (added as entry in
    doc/news/_preparation_next_release.md which contains *(my name)*)
    Please always add something to the the release notes.
  • Longer descriptions should be in documentation or in design decisions.
  • Describe details of how you changed the code in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, should be in the commit messages.

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.

  • I added unit tests
  • I ran all tests locally and everything went fine
  • affected documentation is fixed
  • I added code comments, logging, and assertions (see doc/CODING.md)
  • meta data is updated (e.g. README.md of plugins)

@markus2330
Copy link
Contributor

Thank you for looking at it!

@KurtMi
Copy link
Contributor Author

KurtMi commented Feb 18, 2019

jenkins build all please

@markus2330
Copy link
Contributor

What about removing the semlock plugin as it does not seem to be easy to add? Or should we wait until the global plugins are reworked?

@KurtMi
Copy link
Contributor Author

KurtMi commented Apr 29, 2019

@markus2330 I removed the semlock plugin

@KurtMi KurtMi changed the title Plugins: semlock: enable it Plugins: semlock: remove it Apr 29, 2019
Copy link
Contributor

@markus2330 markus2330 left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! The problem with the Debian package should be fixed now.


removed due to:

- constant pain
Copy link
Contributor

Choose a reason for hiding this comment

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

Was it because of the problems in hooks, because of OS-specific problems, or something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Manly OS-specifics /dev/shm should be mounted as tempfs for example. But I also see no reason for the semlock, the main reason was the race test, but the timestamp consistency check between KdbGet and KdnSet made the race test broken by design.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you are right. The semlock plugin alone cannot fix the race tests.

@@ -71,8 +71,6 @@ logging API:

## Others

fix semlock plugin
Copy link
Contributor

Choose a reason for hiding this comment

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

change to "implement semlock plugin"

@markus2330
Copy link
Contributor

I also retriggered the mac build jobs.

@sanssecours
Copy link
Member

Please rebase this PR on the master branch. Without the changes of commit 483752d the source code formatting checks on Cirrus CI will always fail otherwise.

@KurtMi
Copy link
Contributor Author

KurtMi commented May 3, 2019

jenkins build all please

@markus2330 markus2330 merged commit b5e592a into ElektraInitiative:master May 5, 2019
@markus2330
Copy link
Contributor

@KurtMi thank you very much! I hope we will see further contributions from you!

@KurtMi KurtMi deleted the enable-semlock branch May 5, 2019 10:27
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.

None yet

3 participants