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

cache openssl download #50

Merged
merged 2 commits into from
Jun 22, 2016
Merged

cache openssl download #50

merged 2 commits into from
Jun 22, 2016

Conversation

chipitsine
Copy link
Contributor

@chipitsine chipitsine commented May 23, 2016

(*) cache openssl download when compiling in Travis CI
(*) stop compilation on every warning

@@ -39,7 +39,7 @@ dist_doc_DATA = \
COPYING

AM_CPPFLAGS = $(OPENSSL_CRYPTO_CFLAGS) -D_UNICODE
AM_CFLAGS = -municode
AM_CFLAGS = -municode -Werror
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have -Wall and -Wextra turned on, adding -Werror to that could become very limiting. The code wont compile until all warnings are eliminated is not where I would like to go... Let's see what others say.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi,

On Mon, May 23, 2016 at 01:45:58PM -0700, Selva Nair wrote:

@@ -39,7 +39,7 @@ dist_doc_DATA =
COPYING

AM_CPPFLAGS = $(OPENSSL_CRYPTO_CFLAGS) -D_UNICODE
-AM_CFLAGS = -municode
+AM_CFLAGS = -municode -Werror

We have -Wall and -Wextra turned on, adding -Werror to that could become very limiting. The code wont compile until all warnings are eliminated is not where I would like to go... Let's see what others say.

Looking at warnings and getting rid of the reasonable ones is good :-)

Elimitinating all warnings might just not be possible with mingw, if the
header files itself trigger warnings (which I've seen compiling on
ubuntu 14), so -Werror on-by-default for cross-compiled code is problematic.
So I'd not go there.

gert

USENET is not the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany gert@greenie.muc.de
fax: +49-89-35655025 gert@net.informatik.tu-muenchen.de

@chipitsine
Copy link
Contributor Author

it is being built in Travis-CI, there's Ubuntu 14, I do not see any warning in headers. can you please provide details on that ?

@selvanair
Copy link
Collaborator

Even if the current code can be easily made to pass -Werror doesn't mean that it won't limit us in future. A little more self-discipline to scan through any warnings before submitting a PR is what I would aim for -- that remark is aimed at myself :)

@leobasilio
Copy link
Contributor

I agree with @selvanair.

@cron2
Copy link
Contributor

cron2 commented May 24, 2016

hi,

On Tue, May 24, 2016 at 07:13:38AM -0700, Leonardo Basilio wrote:

I agree with @selvanair.

+1

gert

USENET is not the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany gert@greenie.muc.de
fax: +49-89-35655025 gert@net.informatik.tu-muenchen.de

@selvanair
Copy link
Collaborator

selvanair commented May 24, 2016

So that's it then. @chipitsine : could you please remove the second patch (-Werror) and force-push? A more meaningful commit summary (headline) would be useful too. Thanks.

@chipitsine chipitsine changed the title couple of improvements cache openssl download May 24, 2016
@selvanair
Copy link
Collaborator

Thanks for the edits.
While this may be a good start to implement caching, downloading takes hardly any time or resource these days. it would be much more useful if the build results of openssl (libs and includes) could be cached as well. That is likely where bulk of the re-build time is spent. Would it be too hard to cache those results?

Otherwise I leave it to @mattock to decide whether these changes look good.

@chipitsine
Copy link
Contributor Author

I'm playing with ccache in my private branch. No luck yet, ccache is tricky in case of mingw.

@cron2
Copy link
Contributor

cron2 commented May 27, 2016

Hi,

On Thu, May 26, 2016 at 01:00:08PM -0700, Selva Nair wrote:

Thanks for the edits.
While this may be a good start to implement caching, downloading takes hardly any time or resource these days. it would be much more useful if the build results of openssl (libs and includes) could be cached as well. That is likely where bulk of the re-build time is spent. Would it be too hard to cache those results?

The "normal" openvpn-build (not the gui side) already can do caching
for dependencies. It's a bit complicated (as far as I remember you have
to tell it "now build and cache dependencies!" and later on "use the
precompiled dependencies!") but does work.

Need to look up how this worked and comment here... early next week.

gert

USENET is not the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany gert@greenie.muc.de
fax: +49-89-35655025 gert@net.informatik.tu-muenchen.de

@chipitsine
Copy link
Contributor Author

I had a look at that. it seem to be complicated on openssl version upgrade.
however, I keep it in mind.

if I won't be lucky with ccache, I'll use that

@chipitsine
Copy link
Contributor Author

actually, there's very little need of using openssl in openvpn-gui. it is used only for certificate manipulation, i.e. to encrypt or decrypt it.

it could be done with wincrypt api, I think.

@selvanair
Copy link
Collaborator

Yes, openssl is used only for changing the private key password (a rarely used feature?), replacing it with windows crypto api may be a good idea. That would also eliminate all those queries we often get about cross-compiling openssl.

@chipitsine
Copy link
Contributor Author

I will adress that in separate PR

@mattock
Copy link
Member

mattock commented May 30, 2016

Code-vise this PR looks reasonable, but I would rather focus on using a pre-built OpenSSL version, as that would save a lot more time. We can host the pre-built OpenSSL tar.gz on a webserver if needed.

@chipitsine
Copy link
Contributor Author

compiling openssl takes less than 2 minutes (which is very small time comparing to travis-ci time limit, which is 50 minutes).

also, travis-ci cloud is known to "network name not resolved" or "connection refused" errors pretty often, so I would not like to depend on downloading openssl cache from build.openvpn.net

this patch prevent us from network outages (downloading openssl from openssl.org)

@mattock
Copy link
Member

mattock commented May 30, 2016

Ah, I see. In case of a problematic network this PR could be useful. When building Windows installers for OpenVPN the "compile OpenSSL" step is by far the heaviest. Then again, the build computer I use is pretty low-spec.

@chipitsine
Copy link
Contributor Author

can someone merge it?

@mattock
Copy link
Member

mattock commented Jun 20, 2016

@cron2, @selvanair : any further comments on this? If not, I'll merge it.

@selvanair
Copy link
Collaborator

@mattock : no further comments from me..

@mattock mattock merged commit c77c720 into OpenVPN:master Jun 22, 2016
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.

5 participants