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

CI: Port Windows build to Azure Pipelines #7757

Merged
merged 6 commits into from
Mar 27, 2020
Merged

Conversation

JohnHolmesII
Copy link
Contributor

@JohnHolmesII JohnHolmesII commented Mar 11, 2020

This is the beginning work of a port from Appveyor to Azure. It does not change compilers for windows.

It will build and package an artifact, however there are several things that need to be done before this can be merged:

  • Move all the inline scripts into a dedicated file, a la the linux builds
    Unify the yaml itself so Windows is inside the matrix
    Fix the caching system. This is desperately needed.
  • Fix the experimental build warning

Overall it looks promising. The actual build steps for clang, gcc, and msvc are all the same. Hopefully when all the caching is set up, we can get 5 minute CI.

Optional steps:
Remove travis

  • Remove appveyor

@JohnHolmesII
Copy link
Contributor Author

@hcorion Have a look please.

@Nekotekina
Copy link
Member

Travis still wins by speed (more correct cache inheritance), so there is no need to remove it.
About appveyor, I'm not so sure.

@JohnHolmesII
Copy link
Contributor Author

Well there's something fishy afoot. There must be something wrong with the way the Azure is configured on RPCS3's end. On mine, the linux caching is excellent and beats out travis. You can see it here. Windows builds are at least faster than appveyor, so we can yeet that into the sun.

.travis/setup-windows.sh Show resolved Hide resolved
.travis/setup-windows.sh Outdated Show resolved Hide resolved
.travis/setup-windows.sh Outdated Show resolved Hide resolved
.travis/setup-windows.sh Outdated Show resolved Hide resolved
.travis/setup-windows.sh Outdated Show resolved Hide resolved
azure-pipelines.yml Outdated Show resolved Hide resolved
@JohnHolmesII JohnHolmesII changed the title [WIP] CI: Port Windows build to Azure Pipelines CI: Port Windows build to Azure Pipelines Mar 22, 2020
@JohnHolmesII
Copy link
Contributor Author

JohnHolmesII commented Mar 22, 2020

Well, I think due to the issues with RPCS3's azure pipeline being temperamental, the other changes I had planned are best reserved for another time. In addition, there is a dearth of options available for build caching on Windows. I'm looking into something rather hacky, but even if it works, it's best reserved for another PR. Azure is at least as fast as appveyor was, and is usually faster, so I think the PR is mergeable as is at this point.

@JohnHolmesII JohnHolmesII marked this pull request as ready for review March 22, 2020 23:26
@hcorion
Copy link
Member

hcorion commented Mar 22, 2020

Nice work!
If you're removing Appveyor you also need to add the GitHub Releases setup to azure.

@AniLeo
Copy link
Member

AniLeo commented Mar 22, 2020

If you remove Appveyor I need to update the website as well so don't merge before that

Edit: No update was needed, I fixed what I was misremembering some time ago

@JohnHolmesII JohnHolmesII changed the title CI: Port Windows build to Azure Pipelines [WIP] CI: Port Windows build to Azure Pipelines Mar 23, 2020
@JohnHolmesII
Copy link
Contributor Author

@hcorion Wow, thanks for reminding me, I completely forgot. Deployment is set up and working.

@Nekotekina In order for deployments to work, Azure will need more permissions. In azure pipelines, select the rpcs3 pipeline and there should be a prompt that the pipeline needs "updated permissions" in order to run.

@JohnHolmesII JohnHolmesII changed the title [WIP] CI: Port Windows build to Azure Pipelines CI: Port Windows build to Azure Pipelines Mar 23, 2020
@RipleyTom
Copy link
Contributor

RipleyTom commented Mar 23, 2020

Need to add cacert.pem download to the windows artifact.

@Nekotekina
Copy link
Member

@JohnHolmesII Can you just use the same github connection as llvm-mirror?
It's called RPCS3-Token

@JohnHolmesII
Copy link
Contributor Author

Well, there seems to be a bug in the way Azure handles variables set from scripts. Even simple strings get corrupted with a mysterious roaming apostrophe, which is nondeterministic. I will likely need to manually scrub vars that are passed around. Do not merge until this issue is solved, or else we could corrupt various stages of deployment.

@AniLeo AniLeo self-requested a review March 23, 2020 21:38
@Nekotekina
Copy link
Member

Do does it technically work now?

@JohnHolmesII
Copy link
Contributor Author

@Nekotekina The pr works sometimes. Any environment variables exported can sometimes be corrupted by a single quote. It is random. Look at the Artifact for the windows build, a single quote is in the filename. SOmetimes the single quote shows up in the "BRANCH" variable, which breaks compiliation, since it is expanded by macros in main_window.cpp. I think I'll either have to report and wait for a response from Azure, or do a workaround by making that code read in a file or something. I already put in a workaround for the GitHub release description, but there is work needed for others.

Copy link
Member

@AniLeo AniLeo left a comment

Choose a reason for hiding this comment

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

No constraints on website for this change

Copy link
Member

@hcorion hcorion left a comment

Choose a reason for hiding this comment

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

LGTM, once the corruption issues get fixed upstream 👍

@JohnHolmesII
Copy link
Contributor Author

Alright, well I've worked around all but one of the side effects. I think the changes to main_window are even for the best: rpcs3 version checking should be unified imo.

@AniLeo The "all but one" just happens to be something related to the release artifacts. The title of the release will have an apostrophe at the end, as seen here. The description part (that has the hash and filesize) is properly protected against the bug, so it should be easier to parse. I think it should be easy to scrape the title tho and simply discard the final apostrophe, but I don't know the details of that on the website's end.

Other than that, I think is should be squared away.

rpcs3/rpcs3qt/main_window.cpp Outdated Show resolved Hide resolved
rpcs3/rpcs3qt/main_window.cpp Outdated Show resolved Hide resolved
@AniLeo
Copy link
Member

AniLeo commented Mar 26, 2020

As the website is right now it will work fine since it uses the tag name in URL which is correct, but you may want to fix that release name either way

@JohnHolmesII
Copy link
Contributor Author

@Nekotekina It doesn't appear that the llvm libs have sha256 attatched for windows. Can you add that so I can make the Windows build automatically redownload based on checksum? Same thing with glslang.

The STRINGIZE macro was a little awkward, and difficult to control
at configure time. Since other version information is already
included, the full branch name is now added as a function. It's
runtime instead of compile-time checking, but it seems worth it.
Previously, there was no way of forcing a re-download
of cached dependencies when they were replaced by new ones. In
addition, there was really no verification of downloads or cache.
Now, changing a few lines at the top of the file will automagically
force a cache update.
@JohnHolmesII
Copy link
Contributor Author

Well, in the mean time I simply inlined the hash for now. The script can always be changed later I suppose.

In addition, I made changes to how caching dependencies is done. The previous setup was rather poor, but now it is quite robust.

@Nekotekina Nekotekina merged commit 70d6a12 into RPCS3:master Mar 27, 2020
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.

None yet

8 participants