Skip to content

vcpkg manifest mode#4432

Closed
Be-ing wants to merge 4 commits intomainfrom
vcpkg_manifest
Closed

vcpkg manifest mode#4432
Be-ing wants to merge 4 commits intomainfrom
vcpkg_manifest

Conversation

@Be-ing
Copy link
Copy Markdown
Contributor

@Be-ing Be-ing commented Oct 16, 2021

successor of #4281 using a branch on the upstream repo so GITHUB_TOKEN gets write permission

@github-actions github-actions bot added the build label Oct 16, 2021
@Be-ing Be-ing mentioned this pull request Oct 16, 2021
@Be-ing Be-ing force-pushed the vcpkg_manifest branch 6 times, most recently from 724bc0f to b028f63 Compare October 16, 2021 08:05
Copy link
Copy Markdown
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

While I acknowledge that using submodule is part of the vcpkg examples, we schould verify if this is a good fit for Mixxx.

The submodul solution works probably OK for GitHub actions, but has some usability issues when used localy.
Especially if you are using git submodules to switch easily between different frature branches. A loose reference should be more convenient and become the default locally.

Once the vcpkg configuration is settled, I don't expect many changes.
I also expect, that you can use always the most recent version, even if you are working on an old feature branch.So a strict commit hash dependency is IMHO not helpfull.

CMake already checks the version of the environment at a library basis. We need to make sure this is sufficient anyway becaues we also want to build against system libraries.
If we think that is not sufficient, we may add a semantic version for the vcpkg environment.

Proposal:
cmake may checkout the most recent vcpkg for the current development branch into, lets say a vcpkg-2.4 folder, sibiling to the Mixxx .git folder in the base working tree (the one with the real .git folder in case of git worktrees)
This way the user can have two or more vcpkg environments parallel and is able to freely create worktrees of a stable Mixxx branch or main.
There will be no hassle with submodules and the source tree will be free of files that may confuse a Linux package maintainer.

What do you think?

We should discuss this and other options before merging this.

Comment thread .github/workflows/build.yml
Comment thread tools/debian_buildenv.sh Outdated
Comment thread .github/workflows/build.yml Outdated
Comment thread .github/workflows/build.yml
Comment thread CMakeLists.txt Outdated
Comment thread CMakeLists.txt Outdated
Comment thread vcpkg.json
@Be-ing Be-ing force-pushed the vcpkg_manifest branch 4 times, most recently from 0a137e8 to 499ce62 Compare October 16, 2021 16:30
This was referenced Oct 16, 2021
@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Oct 16, 2021

vcpkg is designed to be used as a submodule. I will not hack around this and don't really have energy to bikeshed this. Working against how tools are designed is asking for trouble.

@Be-ing Be-ing force-pushed the vcpkg_manifest branch 3 times, most recently from 68978f9 to 0289e18 Compare October 16, 2021 17:35
@Be-ing Be-ing changed the title vcpkg manifest mode vcpkg manifest mode + Qt6 builds for macOS & Windows Oct 16, 2021
@Be-ing Be-ing force-pushed the vcpkg_manifest branch 3 times, most recently from 0a85f2d to 66159de Compare October 16, 2021 18:03
@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Oct 20, 2021

@JoergAtGithub would you be okay with merging this with the hack of deleting the too new version of CMake on GitHub Actions for Windows to get vcpkg to download its own version of CMake? We can remove that hack when my upstream PR is merged and released.

@Be-ing Be-ing force-pushed the vcpkg_manifest branch 2 times, most recently from 6cb2cf4 to 4176c75 Compare October 20, 2021 21:37
@JoergAtGithub
Copy link
Copy Markdown
Member

Sorry, but I don't think this is reliable enough yet, to be merged:
1.) It works only with Englisch language pack installed (Remove -ForceEnglishOutput might help)
2.) I needed to change Windows Registry settings to enable TLS1.2 transfer required by the NuGet server (Only Win7)
3.) I couldn't rebuild wihout deleting the packages directory

Also other users like @JosepMaJAZ should test it. In the moment we just know, that it works with combination of Win7, latest VisualStudio update 16.11.5 and Englisch Language pack in addition to the German (which is the default download).

@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Oct 20, 2021

1.) It works only with Englisch language pack installed (Remove -ForceEnglishOutput might help)

I just removed that CLI option for NuGet and it seems to be working. I don't know if that's the source of the problem you ran into. But this is a small issue that could be addressed by documenting that the English language pack needs to be installed.

2.) I needed to change Windows Registry settings to enable TLS1.2 transfer required by the NuGet server (Only Win7)

Considering this only affects Windows 7, I doubt anyone else will be affected by this.

3.) I couldn't rebuild wihout deleting the packages directory

That is annoying and I don't know why that happened. I have never encountered that issue on macOS or Linux. I would not be surprised if it is another issue specific to Windows 7.

Also other users like @JosepMaJAZ should test it.

I agree, it would be good to have more testing.

@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Oct 20, 2021

@JoergAtGithub how exactly could we explain how to install the English language pack in BUILDING.md?

@Be-ing Be-ing force-pushed the vcpkg_manifest branch 4 times, most recently from ddecfa0 to eb77487 Compare October 24, 2021 18:58
@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Oct 24, 2021

I added a sentence to BUILDING.md about needing the English language pack for Visual Studio on Windows.

I am still waiting for review for microsoft/vcpkg-tool#223

@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Nov 17, 2021

With microsoft/vcpkg#21471 and the new --x-abi-tools-use-exact-versions option, I think the issue with cache misses on local builds due to different CMake and PowerShell versions is finally fixed. @JoergAtGithub please test building this branch again to confirm that vcpkg hits the cache.

Once mixxxdj/vcpkg#33 is merged I will switch the vcpkg submodule to the mixxxdj/2.4 branch and I think we can finally merge this.

@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Nov 18, 2021

I confirm the cache miss issues are solved now. I built locally on macOS and vcpkg hit the cache. Then I upgraded CMake with Homebrew, deleted the CMake build directory, and vcpkg still hit the cache.

Copy link
Copy Markdown
Member

@ywwg ywwg left a comment

Choose a reason for hiding this comment

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

I think the submodule method is the right way to go. This PR contains a lot of changes to a lot of platforms though so it needs pretty extensive hand-testing.

Comment thread BUILDING.md
@@ -0,0 +1,162 @@
# Mixxx Build Documentation
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

usually a file like this is called COMPILING.md but this is a nitpick

@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Nov 21, 2021

I took another look at how Tenacity is set up to automatically configure read-only access to GitHub Packages. The trick is pretty simple; just split the GitHub personal access token into two strings so GitHub doesn't automatically deactivate it. This allows for simplifying the documentation; there is no need to run the nuget command manually.

@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Nov 22, 2021

I have tested this on Windows and I am happy to report that it Just Works, both the command line instructions in BUILDING.md and loading the repository in Visual Studio (VSCode works too, but the CMake generator needs to specifically be set to Ninja). Aside from installing the required tools, there are no longer any extra steps on any OS; CMake and vcpkg take care of everything. This is the first time I've been able to compile Mixxx locally on Windows. Mixxx's entire dependency graph is now buildable locally as well, which makes it significantly easier to work on packaging dependencies locally. I do not think we will have Windows developers failing to get started because of confusion running the old batch script. Plus, this makes Mixxx buildable on ARM macOS (as an x86-64 executable).

All that is left to do is merge mixxxdj/vcpkg#33 then switch the vcpkg submodule on this branch to the mixxxdj/2.4 branch.

@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Nov 22, 2021

I have extracted some generally useful code to initialize vcpkg into a CMake module and opened a PR to share it upstream: microsoft/vcpkg#21591

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

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants