Opened 3 months ago

Closed 4 weeks ago

#25549 closed enhancement (implemented)

Add tor CI config for AppVeyor

Reported by: isis Owned by: saper
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-ci, tor-testing, 034-roadmap-subtask, 034-triage-20180328, 034-included-20180328
Cc: catalyst, saper@… Actual Points:
Parent ID: #25550 Points: 2
Reviewer: isis, catalyst Sponsor: Sponsor3

Description

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 #22636): 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.

Child Tickets

TicketTypeStatusOwnerSummary
#25942defectclosedFix win32 test failure for crypto/openssl_version
#25943defectclosedsaperInvestigate and fix tortls/x509_cert_free test failure on win32 CI builds
#25944defectclosedsaperInvestigate and fix tortls/context_new test failure on win32 CI builds

Attachments (2)

64bit.txt (992.8 KB) - added by saper 3 months ago.
64bit build log
32bit.txt (991.8 KB) - added by saper 3 months ago.
32bit build log

Download all attachments as: .zip

Change History (43)

comment:1 Changed 3 months ago by catalyst

Parent ID: #25550

comment:2 Changed 3 months ago by catalyst

Sponsor: Sponsor3

comment:3 Changed 3 months ago by saper

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

comment:4 Changed 3 months ago by saper

Owner: set to saper
Status: newassigned

comment:5 Changed 3 months ago by nickm

Keywords: 034-roadmap-subtask added

comment:6 Changed 3 months ago by nickm

Keywords: 034-triage-20180328 added

comment:7 Changed 3 months ago by nickm

Keywords: 034-included-20180328 added

Changed 3 months ago by saper

Attachment: 64bit.txt added

64bit build log

Changed 3 months ago by saper

Attachment: 32bit.txt added

32bit build log

comment:8 Changed 3 months ago by 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.

comment:9 Changed 3 months ago by nickm

Status: assignedneeds_review

comment:10 Changed 3 months ago by isis

Points: 2
Reviewer: isis, catalyst
Status: needs_reviewneeds_revision

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.

comment:11 Changed 3 months ago by saper

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.

comment:12 Changed 3 months ago by 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.

comment:13 Changed 2 months ago by nickm

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

comment:14 in reply to:  13 ; Changed 8 weeks ago by 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.

comment:15 in reply to:  14 Changed 8 weeks ago by isis

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.

comment:16 Changed 8 weeks ago by nickm

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

comment:17 Changed 8 weeks ago by nickm

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

comment:18 Changed 8 weeks ago by isis

Status: needs_revisionneeds_review

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 (#25942), tortls/x509_cert_free (#25943), and tortls/context_new (#25944).

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

comment:19 Changed 7 weeks ago by saper

Cc: saper@… added

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

comment:20 Changed 7 weeks ago by saper

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

comment:21 Changed 7 weeks ago by 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

Last edited 7 weeks ago by saper (previous) (diff)

comment:22 Changed 7 weeks ago by 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)

comment:23 in reply to:  20 Changed 7 weeks ago by isis

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?

comment:24 in reply to:  22 Changed 7 weeks ago by isis

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 <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)


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!

comment:25 Changed 7 weeks ago by 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)

comment:26 in reply to:  25 Changed 7 weeks ago by isis

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?

comment:27 Changed 7 weeks ago by saper

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.

comment:28 Changed 6 weeks ago by isis

Status: needs_reviewneeds_revision

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

comment:29 Changed 6 weeks ago by saper

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()

:)

Last edited 6 weeks ago by saper (previous) (diff)

comment:30 Changed 6 weeks ago by saper

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

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

comment:31 Changed 6 weeks ago by nickm

Status: needs_revisionneeds_review

comment:32 Changed 6 weeks ago by catalyst

Status: needs_reviewneeds_revision

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

comment:33 in reply to:  18 Changed 6 weeks ago by saper

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 successful build:

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

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

Last edited 5 weeks ago by catalyst (previous) (diff)

comment:34 Changed 5 weeks ago by 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.

comment:35 in reply to:  34 Changed 5 weeks ago by catalyst

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 #26076? 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).

comment:36 Changed 5 weeks ago by nickm

I didn't run into #26076, so it might still be there.

comment:37 Changed 4 weeks ago by nickm

Status: needs_revisionneeds_review

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

comment:38 in reply to:  37 Changed 4 weeks ago by saper

Thanks nickm, much appreciated. I have added my rather minor comments to GitHub.

comment:39 Changed 4 weeks ago by nickm

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 #26076

comment:40 Changed 4 weeks ago by isis

Status: needs_reviewmerge_ready

LGTM. I don't know what to do about #26076 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.

comment:41 Changed 4 weeks ago by nickm

Resolution: implemented
Status: merge_readyclosed

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

Note: See TracTickets for help on using tickets.