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 Travis CI to build MinGW packages #3706

Merged
merged 13 commits into from Jul 12, 2018

Conversation

Projects
None yet
8 participants
@liushuyu
Contributor

liushuyu commented May 1, 2018

As it was said before, this is precedent to the PR #3649

This one maybe controversial, so I expect it will take time to be reviewed and merged.

There's a workaround in the build script, the reason it was here was that libressl module wasn't up-to-date in citra main repository.

Libraries used in this build:

Lib Version
Qt 5 5.9.6
SDL2 2.0.7
libsamplerate 0.1.9

Test build download link is shown here: https://travis-ci.org/liushuyu/citra/jobs/373341674#L4134

The download speed maybe very slow, and it will be expired after 14 days (2018-05-13)


This change is Reviewable

@cluezbot

This comment has been minimized.

cluezbot commented May 1, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-05-01T05:27:49Z: 9c65a45...liushuyu:e4128f616931bf89479fcc479884dd3704a3cb65

@liushuyu

This comment has been minimized.

Contributor

liushuyu commented May 1, 2018

And maybe after this PR is merged, we can remove the MinGW build from Appveyor to cut overall CI time down to around 20 minutes.

Another side effect observed was that UI components now follows native components theme, the binary size shrieked to 15 MB compressed, 65.8 MB uncompressed

@jroweboy

overall looks good. i plan to manually test this out before putting it on canary. I just left a few questions and comments, nothing show stopping.

apt-get update
apt-get install -y software-properties-common wget git python3-pip ccache g++-mingw-w64-x86-64 gcc-mingw-w64-x86-64 mingw-w64-tools
# temporary workaround

This comment has been minimized.

@jroweboy

jroweboy May 1, 2018

Member

we did merge the update pr to the externals repo. If we need to update it again, feel free to send a pr and i'll make sure it gets merged soon :)

This comment has been minimized.

@liushuyu

liushuyu May 1, 2018

Contributor

The problem is the main repository is unaware of the changes of the submodule.
You need to update the submodule in the main repository and commit the change. (b/c the submodule works like a pointer, you need to update the address of that pointer)

This comment has been minimized.

@jroweboy

jroweboy May 1, 2018

Member

yes, and thats something I was expecting you would do as part of this pull request

# !temporary workaround
add-apt-repository ppa:tobydox/mingw-w64 -y
# HACK: the repository does not contain necessary packages for 18.04, we'll use packages for 17.10 for now

This comment has been minimized.

@jroweboy

jroweboy May 1, 2018

Member

i take it that this works, but its kinda surprising to me that it does :)

This comment has been minimized.

@liushuyu

liushuyu May 1, 2018

Contributor

The directory structures for MinGW toolchain stayed the same and they are not binaries for the host system, so that's kinda easy to explain why this worked

export PATH=/citra/cmake/cmake-3.10.1-Linux-x86_64/bin:$PATH
# fix a problem in current MinGW headers
wget -q https://github.com/Alexpux/mingw-w64/raw/master/mingw-w64-headers/crt/errno.h -O /usr/x86_64-w64-mingw32/include/errno.h

This comment has been minimized.

@jroweboy

jroweboy May 1, 2018

Member

i'd rather freeze this to a specific commit instead of master just in case it changes before appveyor decides to update msys2 again.

This comment has been minimized.

@liushuyu

liushuyu May 1, 2018

Contributor

I will pinpoint a specified commit

for i in package/*.exe; do
# we need to process pdb here, however, cv2pdb
# does not work here, so we just simply strip all the debug symbols

This comment has been minimized.

@jroweboy

jroweboy May 1, 2018

Member

this seems a little regrettable, as we were planning on using the debug symbols with minidump to get stacktraces. i don't think we should require symbols, but if someone has any good ideas on how to address this, i'd like to hear them :)

This comment has been minimized.

@liushuyu

liushuyu May 1, 2018

Contributor

I will try to see if cv2pdb can be ported to the build environment.

This comment has been minimized.

@liushuyu

liushuyu May 2, 2018

Contributor

Okay, cv2pdb contains enormous Windows specific code and even MS Assembly.

but if someone has any good ideas on how to address this, i'd like to hear them :)

Well, I don't know if you are interested in Google Breakpad, the crash reporting system used in Chromium/Chrome

cd ..
mkdir package
QT_PLATFORM_DLL_PATH='/usr/x86_64-w64-mingw32/lib/qt5/plugins/platforms/'

This comment has been minimized.

@jroweboy

jroweboy May 1, 2018

Member

not sure right now, but just double checking that the platforms directory for this qt only has qwindows.dll and none of the other platform plugins, correct?

This comment has been minimized.

@liushuyu

liushuyu May 1, 2018

Contributor

There are several files under platform directory. qwindows.dll and qminimal.dll to name a few.

This comment has been minimized.

@jroweboy

jroweboy May 1, 2018

Member

as i recall, we only need qwindows.dll at the moment, so maybe just hard code that.

This comment has been minimized.

@liushuyu

liushuyu May 1, 2018

Contributor

as i recall, we only need qwindows.dll at the moment, so maybe just hard code that.

Okay, I'll hard code that

This comment has been minimized.

@wwylele

wwylele May 1, 2018

Member

Though #3566 is probably introducing some new platform plugins.

This comment has been minimized.

@liushuyu

liushuyu May 2, 2018

Contributor

Though #3566 is probably introducing some new platform plugins.

No it only adds mediaservice plugins

Fortunately, the PPA I used contains QtMultiMedia libraries for MinGW, it should not be a problem.

I wonder what else need to be done to push forward this PR?

results = []
pe = pefile.PE(file_name, fast_load=True)
pe.parse_data_directories()

This comment has been minimized.

@jroweboy

jroweboy May 1, 2018

Member

i'm not too familiar with the PE library used in python here, but is there a way we can replicate how we are detecting system dlls like we currently do here:

# Is this a system dll file?
this method of hardcoded system dlls and checking for 32.dll seems just a little fragile.

This comment has been minimized.

@liushuyu

liushuyu May 1, 2018

Contributor

This PE library is only a static analysis library. Remember that Appveyor is using a Windows (R) Operating system and dumpbin uses dynamic analysis. So the method described in citra/.appveyor/FindDependencies.ps1does not work here since it does not have the concept of WIN32 SystemRoot or something like that.

This comment has been minimized.

@liushuyu

liushuyu May 1, 2018

Contributor

this method of hardcoded system dlls and checking for 32.dll seems just a little fragile.

As mentioned before, but just to clarify a bit more, since the host is not Windows, obviously these dlls are not coming from C:\ of course.

@cluezbot

This comment has been minimized.

cluezbot commented May 1, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-05-01T21:33:17Z: 9c65a45...liushuyu:3458e56202cafa7f6044fbac5e44df43596d8aad

@jroweboy

This comment has been minimized.

Member

jroweboy commented May 1, 2018

@liushuyu liushuyu force-pushed the liushuyu:appveyor-cache branch from 3458e56 to 5304677 May 4, 2018

@liushuyu

This comment has been minimized.

Contributor

liushuyu commented May 7, 2018

To whom may concerned, this PR also had addressed the problem in #3716.
The Qt in PPA has statically linked the dll symbols inside the modules.

@jroweboy

Was this intending on replacing the mingw build entirely? If so we should also do something to disable the appveyor build https://github.com/citra-emu/citra/blob/master/appveyor.yml#L16 maybe comment out mingw (or if you are feeling adventurous, rip it out of the file entirely)

mkdir "$REV_NAME"
# get around the permission issues
cp -r package/* "$REV_NAME"

This comment has been minimized.

@jroweboy

jroweboy May 11, 2018

Member

this root package should be named head-mingw/ instead of package/ in order to replace the existing mingw uploads.

This comment has been minimized.

@liushuyu

liushuyu May 11, 2018

Contributor

The packaged directory is actually $REV_NAME. The package directory is a temporary storage, since the files and directory itself are under root user, we can't mv it or change files in it.

This comment has been minimized.

@jroweboy

jroweboy May 11, 2018

Member

Ah my mistake, I thought this was getting the repo tag name "head" "canary" or "nightly" but it looks like thisi s just for the name of the zip.

@liushuyu

This comment has been minimized.

Contributor

liushuyu commented May 11, 2018

Very well, I'll address your comments later today.

@jroweboy

This comment has been minimized.

Member

jroweboy commented May 11, 2018

Since this conflicts with the frontend camera support, I'm planning on merging that one first and rebasing this one off that. My idea is that after merging camera and rebasing this, we can tag this for canary to make sure that everything will still work with the installer and that the transition will be smooth. Are you fine with this?

@liushuyu

This comment has been minimized.

Contributor

liushuyu commented May 11, 2018

Since this conflicts with the frontend camera support, I'm planning on merging that one first… Are you fine with this?

I am okay with this, and the repository contains Qt Multimedia plugins as well. There shouldn't be any problems.

@jroweboy

This comment has been minimized.

Member

jroweboy commented May 11, 2018

@liushuyu qt camera has been merged. When you rebase and get it working in travis mingw, lemme know and i'll tag it for canary

@liushuyu

This comment has been minimized.

Contributor

liushuyu commented May 11, 2018

@liushuyu qt camera has been merged.

Wow, I didn't expect it to be merged this fast.

When you rebase and get it working in travis mingw, lemme know and i'll tag it for canary

I will finish it probably today if nothing goes south. 😄

@liushuyu liushuyu force-pushed the liushuyu:appveyor-cache branch 5 times, most recently from 22f069f to b0b9ba8 May 11, 2018

@liushuyu

This comment has been minimized.

Contributor

liushuyu commented May 11, 2018

Was this intending on replacing the mingw build entirely? If so we should also do something to disable the appveyor build https://github.com/citra-emu/citra/blob/master/appveyor.yml#L16 maybe comment out mingw (or if you are feeling adventurous, rip it out of the file entirely)

Removed in commit b0b9ba8 . We are using a VCS, so I don't comment out things.

Just remember not to do a squash merge and everyone will be happy 😸

@liushuyu liushuyu force-pushed the liushuyu:appveyor-cache branch from b0b9ba8 to c6c3fec May 12, 2018

@jroweboy jroweboy added canary-merge and removed canary-merge labels May 12, 2018

@j-selby

Looking good!

.travis.yml Outdated
@@ -49,6 +49,19 @@ matrix:
services: docker
cache: ccache
script: "./.travis/linux-frozen/build.sh"
- os: linux
env: NAME="linux build (MinGW)"

This comment has been minimized.

@j-selby

j-selby May 13, 2018

Contributor

Feel like the names of the other builds ("linux build") should also be changed to be differentiated, if you are going to be naming them all linux build. Can I suggest that you drop the Linux build stuff (Linux can already be implied) and instead name this something like "cross-compile build (MinGW)"?

This comment has been minimized.

@liushuyu

liushuyu May 13, 2018

Contributor

How about MinGW build straight?

This comment has been minimized.

@j-selby

j-selby May 13, 2018

Contributor

sure, sounds good!

wget -q https://github.com/Alexpux/mingw-w64/raw/d0d7f784833bbb0b2d279310ddc6afb52fe47a46/mingw-w64-headers/crt/errno.h -O /usr/x86_64-w64-mingw32/include/errno.h
mkdir build && cd build
cmake .. -DCMAKE_TOOLCHAIN_FILE="$(pwd)/../CMakeModules/MinGWCross.cmake" -DUSE_CCACHE=ON -DCMAKE_BUILD_TYPE=Release -DENABLE_QT_TRANSLATION=ON -DCITRA_ENABLE_COMPATIBILITY_REPORTING=${ENABLE_COMPATIBILITY_REPORTING:-"OFF"}

This comment has been minimized.

@wwylele

wwylele May 15, 2018

Member

Need to add flag -DENABLE_COMPATIBILITY_LIST_DOWNLOAD=ON do include the compatibility list

This comment has been minimized.

@liushuyu

liushuyu May 15, 2018

Contributor

Need to add flag -DENABLE_COMPATIBILITY_LIST_DOWNLOAD=ON do include the compatibility list

I don't know this option existed... I'll add that now, the options are copypasta from other build jobs.

@wwylele

This comment has been minimized.

Member

wwylele commented May 16, 2018

Just put this here so that we don't lose record:

We have got some report saying that the travis build had worse performance than the appveyor build (of the same canary version, of course). We still don't have idea why it is like this.

Another issue is that audio is broken with audio stretching off in this travis build. It is probably related to the SDL update

@Leo626

This comment has been minimized.

Leo626 commented May 17, 2018

I noticed now that the system camera shows a green screen with this. A lot of dll files are missing including the mediaservice and styles folder. Not sure if that was the cause of this.

Canary-823fe4c (496)

explorer_2018-05-16_22-48-00

Canary-e9bd59d (490)

explorer_2018-05-16_22-50-01

@liushuyu

This comment has been minimized.

Contributor

liushuyu commented May 17, 2018

I noticed now that the system camera shows a green screen with this. A lot of dll files are missing including the mediaservice and styles folder. Not sure if that was the cause of this.

@Leo626 Thanks for figuring out the problem, the mediaservice plugins are now copied. The other DLLs you spot missing were intentional move. These plugins functionalities are now included in Qt5* DLLs.

@zhaowenlan1779

This comment has been minimized.

Member

zhaowenlan1779 commented May 19, 2018

latest canary
image

I guess this was "Citra Canary" before?

And this one?
image

And the citra log.

mkdir package/platforms
cp "${QT_PLATFORM_DLL_PATH}/qwindows.dll" package/platforms/
cp -r "${QT_PLATFORM_DLL_PATH}/../mediaservice/" package/

This comment has been minimized.

@zhaowenlan1779

zhaowenlan1779 Jun 18, 2018

Member

Em. This copies all the dlls to the destination. But in fact that's not necessary. For example this includes a few xxxxxxd.dll, they are only used for Debug and is completely useless for Release builds.

This comment has been minimized.

@liushuyu

liushuyu Jun 19, 2018

Contributor

I'll take a look at this.

This comment has been minimized.

@liushuyu
@liushuyu

This comment has been minimized.

Contributor

liushuyu commented Jun 19, 2018

Okay... I decided not to continue waiting for upstream provider to update SDL 2 since we have cubeb now. I'll still keep track of the update, it may take a while.

I'll address the last few issues here.
And if everything looks good to you, then just merge.

@jroweboy

This comment has been minimized.

Member

jroweboy commented Jun 26, 2018

Lemme know when you get a chance to fix the last few outstanding issues, so we can get this merged

@liushuyu liushuyu force-pushed the liushuyu:appveyor-cache branch from 34e0119 to 3a41f3a Jul 5, 2018

@liushuyu

This comment has been minimized.

Contributor

liushuyu commented Jul 5, 2018

And for #3891 I want to wait for THAT PR to be merged first and I'll match the changes in this PR.

@jroweboy

This comment has been minimized.

Member

jroweboy commented Jul 12, 2018

@liushuyu #3891 is now merged :)

@liushuyu

This comment has been minimized.

Contributor

liushuyu commented Jul 12, 2018

@liushuyu #3891 is now merged :)

Good to know. I'll push a commit to match the changes soon.

liushuyu and others added some commits Apr 12, 2018

travis: MinGW build on Travis CI
... 1. updated submodule libressl
    2. suggestion from @jroweboy
travis: add essential flags to cmake cmdline
added -DENABLE_COMPATIBILITY_LIST_DOWNLOAD=ON
@cluezbot

This comment has been minimized.

cluezbot commented Jul 12, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-07-12T01:34:20Z: edcea90...liushuyu:13049b289dcc4f6296fdfff51914e8bdc7f371a4

@liushuyu liushuyu force-pushed the liushuyu:appveyor-cache branch from 3a41f3a to 13049b2 Jul 12, 2018

@liushuyu

This comment has been minimized.

Contributor

liushuyu commented Jul 12, 2018

After this got merged, #3839 will be updated with changes in this PR

@jroweboy jroweboy merged commit 331c6f4 into citra-emu:master Jul 12, 2018

0 of 3 checks passed

code-review/reviewable 10 files, 11 discussions left (jroweboy, liushuyu, wwylele, zhaowenlan1779)
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@CeruleanSky

This comment has been minimized.

CeruleanSky commented Jul 12, 2018

The appveyor creates an archive that just has the pdb debug symbols for the canary builds, For example for build 632 citra-windows-mingw-20180712-76a3229-debugsymbols.zip. Travis doesn't seem to have a link for those. I was wondering if this could be easily modified to either create the debug symbols artifact and make it downloadable from travis or the github page, duplicating the appveryor behavior. That way it is easier to get better stack back traces, especially for these versions that tend to be the most unstable and in need of the best diagnostics and testing.

@jroweboy

This comment has been minimized.

Member

jroweboy commented Jul 12, 2018

@CeruleanSky Appveyor will continue to be used for validating that builds are working properly from our two windows environments MSVC and MINGW64, and artifacts for debug symbols can still be found there and will still be there for the forseeable future. All this PR does is change which service uploads the build to github

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment