Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

gpgme: working gpgme unit test prototype #2035

Closed
wants to merge 10 commits into from

Conversation

petermax2
Copy link
Member

Affects #896 , #1973 , #2008

Purpose

This is just a unit test that is supposed to run on the build server.

PLEASE DO NOT MERGE!!

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar nothing
needs to be checked.

  • commit messages are fine ("module: short statement" syntax and refer to issues)
  • 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)
  • release notes are updated (doc/news/_preparation_next_release.md)

@markus2330 please review my pull request

@markus2330
Copy link
Contributor

That was really quick! Thank you for testing this!

@petermax2
Copy link
Member Author

petermax2 commented May 31, 2018

@ingwinlu Could you please help me to install ligpgme-dev in the build containers?

@markus2330 I started to work on this last week but did not find time, so far, to complete it.

@petermax2
Copy link
Member Author

I have a running unit test that creates a 1024 bit RSA key by utilizing gpgme. Let's see how the build server responds.

@petermax2
Copy link
Member Author

jenkins build libelektra please

@petermax2
Copy link
Member Author

jenkins build clang please

@petermax2
Copy link
Member Author

@ingwinlu

Trying to acquire lock on [docker-images]
Found 0 available resource(s). Waiting for correct amount: 1.
[docker-images] is locked, waiting...

I may have broken something. Sorry!

@ingwinlu
Copy link
Contributor

That is on purpose. Only a single build is allowed to create docker images at a time (to avoid clogging the build system). This is further refined in #2003. I guess you had old builds running and hence the new ones were blocked. This is also improved in #2003 were new builds auto cancel the old ones.

There should be debian packages on sid and stretch for gpgme-dev: https://packages.debian.org/search?suite=default&section=all&arch=any&searchon=names&keywords=libgpgme-dev

What other questions did you have?

@petermax2
Copy link
Member Author

petermax2 commented May 31, 2018

What other questions did you have?

Everything works 😃 Thank you for your quick response!

@markus2330
Copy link
Contributor

#2003 is merged now.

@ingwinlu Does @petermax2 now need to rebase or will the changes take effect immediately?

to test how the build server responds
@petermax2
Copy link
Member Author

jenkins build libelektra please

@petermax2
Copy link
Member Author

petermax2 commented Jun 1, 2018

So we have a working key generation. GPGME created three 512 bit RSA private keys with sub-keys. 🎉

139/223 Test  #99: testmod_gpgme ............................   Passed   96.54 sec

I wanted to repeat the test more often, but ran into a time-out.

So far, gpgme looks promising. Maybe it would be worth the effort to replace my gpg.c module with gpgme?

@ingwinlu
Copy link
Contributor

ingwinlu commented Jun 1, 2018 via email

@markus2330
Copy link
Contributor

An interesting case would be where the current test cases fail and only gpgme succeeds. Maybe you can run all crypto test cases in a loop until they fail? (while ctest -R crypt; do; done)

And afaik key generation isn't something our plugins do, so I would not test that.

@petermax2
Copy link
Member Author

petermax2 commented Jun 2, 2018

I added the test case, which fails on the build server (for the crypto / fcrypt plugin), which is: importing the Elektra test key. I repeat the test case 20 times. Let's see how the build server responds.

@markus2330
Copy link
Contributor

Unfortunately the build server is down for the weekend.

I think we need to repeat this more than 20 times. Afaik in all build jobs of the last weeks the failure happened only once. And I think you need to repeat the whole test case (with ctest) so that actually a new TMPDIR/HOME is used.

@markus2330 markus2330 mentioned this pull request Jun 2, 2018
14 tasks

gpgme_check_version (NULL);
setlocale (LC_ALL, "");
gpgme_set_locale (NULL, LC_CTYPE, setlocale (LC_CTYPE, NULL));
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not globally change anything (and especially not locales, this might make applications dysfunctional). If we set it, it should only be done for the gpgme context but why set it at all?

It looks like gpgme is engineered properly, as far as I saw it everything seems to be operating only on its context (and never globally, except in gpgme_set_locale if a NULL pointer is passed).

It might be useful to allocate the gpgme context only once (in open), it might be faster then.

Copy link
Member Author

Choose a reason for hiding this comment

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

The gpgme manual recommends to set the locale for initializing gpgme. I am not sure why they recommend it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine for prototyping but probably should not be used like this in any released software.
What stands against creating a gpgme context to solve the issue (and not have to rely on maybe wonky locale)?

Copy link
Contributor

Choose a reason for hiding this comment

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

What stands against creating a gpgme context to solve the issue (and not have to rely on maybe wonky locale)?

It would lead to yet another configuration setting of the plugin. If the locales are broken, pinentries shown by other applications will be broken, too, won't they?

But if we set the locales for the context is a minor thing. I only wanted to note that we must not set it globally.

@ingwinlu
Copy link
Contributor

ingwinlu commented Jun 4, 2018

The build server is currently configured to kill build jobs after a certain time of inactivity on stdout. This is to avoid zombie jobs.

The current key generation takes long enough without producing output that this policy is enforced.

@petermax2
Copy link
Member Author

I close this PR for now and continue the work later on.

@petermax2 petermax2 closed this Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants