Add tor CI config for AppVeyor

At the Rome meeting, it was discussed (apparently, I wasn't there that time) to have an AppVeyor config for tor. As I understand it (and please feel free to correct this ticket!), the idea is to have multiple CI systems running (which is a thing we already do!). For example, currently, we have Jenkins and we also have TravisCI for personal (Github-based) forks (as per legacy/trac#22636 (moved)): Jenkins tests (essentially) (Debian package-based) builds on master and known (supported) tor versions, while Travis tests anything any developer pushes (albeit only for GCC/Clang on Linux, because everything else is unsupported/slow).

We should setup an AppVeyor config for testing tor on Windows. Ideally, it should match the testing behaviour of our Jenkins/Travis builds, so that we don't get spurious errors on one system or another.

changed milestone to %Tor: 0.3.4.x-final in legacy/trac

added 034-included-20180328 in Legacy / Trac 034-roadmap-subtask in Legacy / Trac 034-triage-20180328 in Legacy / Trac component::core tor/tor in Legacy / Trac milestone::Tor: 0.3.4.x-final in Legacy / Trac owner::saper in Legacy / Trac parent::25550 in Legacy / Trac points::2 in Legacy / Trac priority::medium in Legacy / Trac resolution::implemented in Legacy / Trac reviewer::catalyst in Legacy / Trac reviewer::isis in Legacy / Trac severity::normal in Legacy / Trac sponsor::3 in Legacy / Trac status::closed in Legacy / Trac tor-ci in Legacy / Trac tor-testing in Legacy / Trac type::enhancement in Legacy / Trac labels

Trac:
Parent: N/A to legacy/trac#25550 (moved)

Trac:
Sponsor: N/A to Sponsor3

Working on it, got basic compilation using msys64 provided libraries working.

Trac:
Username: saper

Trac:
Username: saper
Status: new to assigned
Owner: N/A to saper

Trac:
Keywords: N/A deleted, 034-roadmap-subtask added

Trac:
Keywords: N/A deleted, 034-triage-20180328 added

Trac:
Keywords: N/A deleted, 034-included-20180328 added

Trac:
Username: saper

64bit.txt

64bit build log

Trac:
Username: saper

Trac:
Username: saper

32bit.txt

32bit build log

Trac:
Username: saper

I managed to compile a working tor binaries using AppVeyor.

appveyor.yml file used: http://repo.or.cz/tor/appveyor.git/blob/6c8ceebc62c6249bc67b9ac4d1481d8f214c718a:/appveyor.yml

There are some errors in tests (full logs attached).

This build does not use recommended compiler flags, I had to add --disable-gcc-hardening to make it work.

Trac:
Username: saper

Trac:
Status: assigned to needs_review

Some comments/review I left in IRC, for the record:

• At the end of the build, if it failed, there will be a test-suite.log file in the topmost build directory describing the failures, so probably we'll want to cat that to stdout.
• Figuring out how to not use --disable-gcc-hardening.
• Instead of make; make install; make test, we should just be able to do make test, I think?
• We should do make check (if possible… it calls all kinds of perl/python/shell) instead of make test.

Trac:
Points: N/A to 2
Reviewer: N/A to isis, catalyst
Status: needs_review to needs_revision

Updated appveyor.yml: http://repo.or.cz/tor/appveyor.git/blob/8807cfb47251059482fea948b5be1d7da60888dc:/appveyor.yml

Build logs on AppVeyor: https://ci.appveyor.com/project/saper/appveyor/build/1.0.44

• make check works
• there is no need to --disable-gcc-hardening, removed
• AppVeyor understands separate build and test phases, therefore make and make install are kept separate from make check

Few things I do not like about the current make check:

• main test produces a lot of sub-tests that are reported as one in the summary (and to find details one needs test-suite.log or test.log file). Simple make test output was much more readable. This can be fixed by using a custom automake test driver.
• Tinytest produces a messy output like:

geoip/load_file: [forking] 
  FAIL ../src/test/test_geoip.c:409: Unable to load geoip from "/c/projects/appveyor/src/config/geoip"
  FAIL ../src/test/test_geoip.c:411: assert(0 OP_EQ rv): 0 vs -1
  [load_file FAILED]

I hope it can be parsed somehow to be displayed in the AppVeyor's test output tab.

Trac:
Username: saper

The remaining error:

crypto/openssl_version: [forking] 
  FAIL ../src/test/test_crypto.c:154: assert(!strcmpstart(version, h_version))
  [openssl_version FAILED]

This is probably related to the MSYS64/MinGW build environment - C:\msys64\mingw32\bin\openssl version is something different than C:\msys64\usr\bin\openssl version. Swapping them in PATH does not help.

Trac:
Username: saper

Hm. Can you get it to dump version and h_version there, to see what they are claiming to be?

Replying to nickm:

Hm. Can you get it to dump version and h_version there, to see what they are claiming to be?

crypto/openssl_version: [forking] openssl version = 1.0.2l
openssl h_version = 1.0.2n
  FAIL ../src/test/test_crypto.c:156: assert(!strcmpstart(version, h_version))
  [openssl_version FAILED]

(From this build.)

I believe the mismatch is due to using pacman to install openssl when it's already installed; I'll try not reinstalling it.

Replying to isis:

Replying to nickm:

Hm. Can you get it to dump version and h_version there, to see what they are claiming to be?

{{{ crypto/openssl_version: [forking] openssl version = 1.0.2l openssl h_version = 1.0.2n FAIL ../src/test/test_crypto.c:156: assert(!strcmpstart(version, h_version)) [openssl_version FAILED] }}} (From this build.)

I believe the mismatch is due to using pacman to install openssl when it's already installed; I'll try not reinstalling it.

Update: It's because pacman is installing libevent, and its version of libevent wants openssl, and somehow it doesn't recognise that openssl is already installed. Also, there's no way to tell pacman which version of openssl to install, because Arch is a steaming pile of crap made by masochists who like everything to be maximally broken all the time. I'm going to try building libevent from git instead, and get rid of this pacman garbage entirely.

fwiw, you can build libevent with --disable-openssl

(That is, Tor doesn't use or need libevent's openssl support.)

There's a bunch of fixes and a bit of a hack to add IRC notifications and other such things in my bug25549 branch. I ended up needed to build libevent from git, so I enabled caching of its build directory between builds (not sure if this works yet, as caching only happens on successful builds). Everything appears to be working, but there's still three tests which fail, which I've created new tickets for: crypto/openssl_version (legacy/trac#25942 (moved)), tortls/x509_cert_free (legacy/trac#25943 (moved)), and tortls/context_new (legacy/trac#25944 (moved)).

My branch includes @saper's patches, which I've reviewed. Please review my additions, thank you!

Trac:
Status: needs_revision to needs_review

Ouch, didn't see there were so many updates! Thank you isis for picking it up! (Now I think I am subscribed to notifications).

Trac:
Username: saper
Cc: catalyst to catalyst, saper@saper.info

I think zstd is not working because, again, there is no zstd-devel available.

Trac:
Username: saper

IRC gateway path should not be hardcoded (http://repo.or.cz/tor/appveyor.git/commitdiff/5a3cbaf3758d6e93ab0e206e6bd88d1001304628) also it would be cool if the notificator didn't assume we all run on Github

Trac:
Username: saper

Funny thing: we need utf-8 support for IRC:)

:charm.oftc.net NOTICE AUTH :*** Looking up your hostname...
:charm.oftc.net NOTICE AUTH :*** Checking Ident
:charm.oftc.net NOTICE AUTH :*** Couldn't look up your hostname
:charm.oftc.net NOTICE AUTH :*** No Ident response
:charm.oftc.net NOTICE appveyor-ci :*** Connected securely via TLSv1.2 ECDHE-RSA-AES256-GCM-SHA384-256
:charm.oftc.net 001 appveyor-ci :Welcome to the OFTC Internet Relay Chat Network appveyor-ci
PRIVMSG #tor-ci :git://repo.or.cz/tor/appveyor.git 0master 5a3cbaf - Marcin Cieślak: tests: do not hardcode path for IRC notifications
ERROR: Failed to send notification: 
Traceback (most recent call last):
  File "C:\projects\appveyor\scripts\test\appveyor-irc-notify.py", line 195, in <module>
    notify()
  File "C:\projects\appveyor\scripts\test\appveyor-irc-notify.py", line 188, in notify
    irc_sock.send('PRIVMSG #{} :{}\r\n'.format(channel, msg).encode())
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc5 in position 81: ordinal not in range(128)

Trac:
Username: saper

Replying to saper:

I think zstd is not working because, again, there is no zstd-devel available.

Yeah, that's definitely not working… maybe chocolatey has it packaged?

Replying to saper:

Funny thing: we need utf-8 support for IRC:)

{{{ :charm.oftc.net NOTICE AUTH :*** Looking up your hostname... :charm.oftc.net NOTICE AUTH :*** Checking Ident :charm.oftc.net NOTICE AUTH :*** Couldn't look up your hostname :charm.oftc.net NOTICE AUTH :*** No Ident response :charm.oftc.net NOTICE appveyor-ci :*** Connected securely via TLSv1.2 ECDHE-RSA-AES256-GCM-SHA384-256 :charm.oftc.net 001 appveyor-ci :Welcome to the OFTC Internet Relay Chat Network appveyor-ci PRIVMSG #tor-ci :git://repo.or.cz/tor/appveyor.git 0master 5a3cbaf - Marcin Cieślak: tests: do not hardcode path for IRC notifications ERROR: Failed to send notification: Traceback (most recent call last): File "C:\projects\appveyor\scripts\test\appveyor-irc-notify.py", line 195, in notify() File "C:\projects\appveyor\scripts\test\appveyor-irc-notify.py", line 188, in notify irc_sock.send('PRIVMSG #{} :{}\r\n'.format(channel, msg).encode()) UnicodeDecodeError: 'ascii' codec can't decode byte 0xc5 in position 81: ordinal not in range(128) }}}

Yeah… that script is a bit janky. The original (at least the one that I took) is here. It definitely doesn't handle UTF-8. :'( I'm sorry it messed up your name!

Don't be sorry, it's fun!

I have pushed some improvements for IRC to my branch on top of your changes. Now trying to fight openssl. Interestingly, without pacman running we still get OpenSSL from somewhere. Jumping into RDP to figure out from where exactly it is coming (looks like mingw has it preinstalled)

Trac:
Username: saper

Replying to saper:

Don't be sorry, it's fun!

I have pushed some improvements for IRC to my branch on top of your changes. Now trying to fight openssl. Interestingly, without pacman running we still get OpenSSL from somewhere. Jumping into RDP to figure out from where exactly it is coming (looks like mingw has it preinstalled)

The Windows VMs that AppVeyor runs also already have OpenSSL 1.0.2l installed, so part of the issue might be that mingw has its own (newer) header installed somewhere?

It's not even that... pacman query shows that MSYS/MinGW come with both OpenSSL's preinstalled as we see them - it is not libevent pulling them in.

I need to spend more time on RDP there and figure out compiler/linker paths. Even if we ask nicely to upgrade it can always happen in the future.

Maybe we should stop amending PATH to prevent loading of a (newer) DLL.

Trac:
Username: saper

Changing this back to needs_revision, since maybe there's something smarter we could do to get a not-mismatched OpenSSL on that system?

Trac:
Status: needs_review to needs_revision

I wonder if that OpenSSL version check is that important, maybe it should be added to the autoconf tests?

I try using --with-openssl-dir option, I hope that one works

Edit: looks like we have the following in configure.ac:

dnl XXXX check for OPENSSL_VERSION_NUMBER == SSLeay()

:)

Trac:
Username: saper

So, it seems, --with-openssl-dir fixed the OpenSSL versioning mismatch issue.

http://repo.or.cz/tor/appveyor.git/commitdiff/eb6231a97d4b864817df51886001aa8a41b28bac

Trac:
Username: saper

Trac:
Status: needs_revision to needs_review

Thanks! I've marked legacy/trac#25942 (moved) as merge_ready; I'm setting this to needs_revision while we work on the other test failures.

Trac:
Status: needs_review to needs_revision

Replying to isis:

I ended up needed to build libevent from git, so I enabled caching of its build directory between builds (not sure if this works yet, as caching only happens on successful builds).

There is a message from AppVeyor after a [build]:

Error calculating dependencies CRC for C:\projects\libevent: Illegal characters in path.

Plus we have a non-deterministic behaviour of test shell scripts (legacy/trac#26067 (moved)[catalyst: corrected ticket number]legacy/trac#26076 (moved)).

Trac:
Username: saper

So, I've rebased the branch onto master, and hacked randomly on it until it passed. It's in my github repository as "appveyor3", and here it is passing: https://ci.appveyor.com/project/nmathewson/tor/build/1.0.8

Does that seem good? If so, I think the next step is to squash it down to a more manageable branch, clean it up a little, and get it reviewed again.

Replying to nickm:

So, I've rebased the branch onto master, and hacked randomly on it until it passed. It's in my github repository as "appveyor3", and here it is passing: https://ci.appveyor.com/project/nmathewson/tor/build/1.0.8

Does that seem good? If so, I think the next step is to squash it down to a more manageable branch, clean it up a little, and get it reviewed again. Thanks! It looks good at first glance. I might want to study it a bit more. Did you figure out how to fix legacy/trac#26076 (moved)? Or is it still (possibly) intermittently failing?

I agree that squashing it into more manageable commits might be a good idea (and easier to review).

I didn't run into legacy/trac#26076 (moved), so it might still be there.

Okay -- as a minimal branch, I propose appveyor_min_034.

It removes some stuff from the branches that might have been a good idea, but turned out not to be necessary for appveyor.

It is built by merging a branch appveyor_min_029 into master -- I think that branch would have potential on 0.2.9, but we would need to backport an array of various unit test fixes from master in order to get it working.

Pull request at https://github.com/torproject/tor/pull/118

Trac:
Status: needs_revision to needs_review

Thanks [comment:37 nickm], much appreciated. I have added my rather minor comments to GitHub.

Trac:
Username: saper

Simplified further per your comments. I've made a new branch called appveyor_min_034_v2 -- PR at https://github.com/torproject/tor/pull/119 .

I still don't have a solution for legacy/trac#26076 (moved)

LGTM. I don't know what to do about legacy/trac#26076 (moved) for now. Perhaps we'll start to see a pattern emerge to the failures, or maybe we'll have to ignore that test on appveyor for now.

Trac:
Status: needs_review to merge_ready

okay. Isis and Catalyst both say "merge", so I'm merging!

Trac:
Resolution: N/A to implemented
Status: merge_ready to closed

closed

changed time estimate to 16h

mentioned in issue legacy/trac#25942 (moved)