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

Use Ubuntu 16.10 in Docker to test on Linux #419

Closed
wants to merge 1 commit into from
Closed

Use Ubuntu 16.10 in Docker to test on Linux #419

wants to merge 1 commit into from

Conversation

angelsl
Copy link
Contributor

@angelsl angelsl commented Mar 20, 2017

Checklist:

  • I have read the CONTRIBUTING document. [REQUIRED]
  • My code follows the code style of this project. [REQUIRED]
  • All new and existing tests passed. [REQUIRED]
  • I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

@angelsl
Copy link
Contributor Author

angelsl commented Mar 20, 2017

This is a strange regression.

FAIL!  : TestCsvParser::testUnicode() 't.at(0).at(0) == "€1"' returned FALSE. ()
   Loc: [/keepassxc/src/tests/TestCsvParser.cpp(332)]

@angelsl angelsl mentioned this pull request Mar 20, 2017
6 tasks
@angelsl
Copy link
Contributor Author

angelsl commented Mar 20, 2017

Fixed that issue by setting the locale (the default locale in Docker's Ubuntu images is POSIX, which basically breaks the moment you have non-ASCII).

Seems to work now.

@phoerious
Copy link
Member

phoerious commented Mar 20, 2017

the problem with this approach is that we won't notice when things break on Ubuntu 14.04 anymore. That's important to be able to build our AppImage later one.
16.10 is also generally too new to be a good base image. Travis should build on the oldest supported platform.

@angelsl
Copy link
Contributor Author

angelsl commented Mar 20, 2017

@phoerious,

You have rejected:

  1. backporting Argon2 and Gcrypt to 14.04, citing maintenance costs
  2. bundling Argon2 and Gcrypt
  3. testing on 16.10, because you need to know when 14.04 breaks and because 16.10 is too new
  4. building Argon2 and Gcrypt in Travis because Travis build times are already too long

and not once have you suggested anything.

I don't exactly agree with all these reasons, but since you are project maintainer it is your decision.

I'm out of ideas (and patience). When you and/or your team can find a way to make these new dependencies available, mention me again if you want me to rebase my changes.

Good luck.

@angelsl angelsl closed this Mar 20, 2017
@angelsl angelsl deleted the travisdocker branch March 20, 2017 16:11
@phoerious
Copy link
Member

phoerious commented Mar 20, 2017

@angelsl I'm not rejecting it, I'm only not yet too happy with the solution. I know it's difficult and I don't have a definite solution myself. I really appreciate your work on this, but finding an optimal solution on this is difficult. I only want to make sure that whatever solution we choose in the end is the best possible. Maybe there is no good solution.
I'm sorry if this looks like I don't appreciate the effort you put into this.

@angelsl
Copy link
Contributor Author

angelsl commented Mar 20, 2017

@phoerious,

OK, fair enough. Thanks for considering my suggestions.

I'll do the necessary rebasing if or when the time comes to merge #399.

@phoerious
Copy link
Member

phoerious commented Mar 20, 2017

@angelsl I'm really thinking hard about a solution. All of them have pros and cons:

  • Backporting (pro: easy to use - con: we need to maintain a PPA)
  • Bundling (pro: relatively low maintenance cost, same version on all platforms - con: doubles or triples the size of our source code)
  • Using 16.10 (pro: just works - con: too new for making it the oldest supported system, building AppImage unsolved*)
  • Building in Travis (pro: clean solution, just works - con: Travis builds will probably take twice the time, i.e. 7-10 minutes or so)

* this is really a problem. Our first AppImage was built on 16.04 (not even 16.10) and we got a lot of reports that it didn't work on users' platforms due to Glibc being too new.

Right now the first and the last option seem to be the best possible, but I'm not too happy with either. :-(
If it were only Argon2, I would go for the second option, but libgcrypt is huge (almost 3MB of source code).

Again, please do not misunderstand my concerns as unnecessary nitpicking. I really want to find a good solution, but we can't make that decision lightly. Whatever we choose has to work seamlessly and be maintained for years.

@angelsl
Copy link
Contributor Author

angelsl commented Mar 20, 2017

@phoerious

The effort needed to maintain a PPA is just about the same as building the dependencies in Travis or bundling them. Action is only needed when there is a new update, and even then the only action needed is to update the sources (because a PPA is built on Ubuntu's servers).

In fact it is possible to request an official backport in which case there is no maintenance needed on your end.

@phoerious
Copy link
Member

phoerious commented Mar 20, 2017

For a PPA, we still need to build stuff in a dedicated 14.04 container (or does Launchpad do all that for you?), whereas a bundled version is just compiled in whatever environment we are. But maybe we can use a separate Travis instance for that. Perhaps a PPA is the best-possible path to go.
A backporting request is also worth trying.

@angelsl
Copy link
Contributor Author

angelsl commented Mar 20, 2017

Launchpad builds the packages from source on their build farm. In fact you cannot simply upload .debs to Launchpad, you have to upload sources.

@phoerious
Copy link
Member

Hmm. Then this actually looks like a solution. I'll have a look into it.

@droidmonkey
Copy link
Member

Perhaps we need to investigate another CI service instead of Travis. If its not meeting our needs, then we should adapt to a more capable platform.

@angelsl
Copy link
Contributor Author

angelsl commented Mar 20, 2017

I've filed backport requests for Argon2 and Gcrypt.

Argon2 backports cleanly to Trusty. The Gcrypt package uses newer Debian build tools than available in Trusty, though, so that fails. Here is the PPA of my attempt. (You can copy packages to a PPA for this project.)

For reference, the backportpackage invocation:

backportpackage -u ppa:angelsl/trusty-crypto-backports -s yakkety -d trusty argon2
backportpackage -u ppa:angelsl/trusty-crypto-backports -s yakkety -d trusty libgcrypt20

@phoerious
Copy link
Member

Amazing. Thank you!

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.

3 participants