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

Switch to manifest mode #80

Merged
merged 9 commits into from
Nov 18, 2023
Merged

Conversation

daschuer
Copy link
Member

This is a prove of concept PR for the vcpkg manifest mode.

Advantages:

  • Upload of single packages to "GitHub packages"
  • Works on the contributors repro.
  • Maintains in place failure notification
  • Optional provides the binary blob, that people can use independent from a GitHub Account.

vcpkg.json Outdated Show resolved Hide resolved
- name: Create buildenv archive
run: ./vcpkg export --vcpkg-root=${{ matrix.vcpkg_path }} --x-all-installed --zip --output=${{ env.DEPS_BASE_NAME }}-${{ env.MIXXX_VERSION }}-${{ matrix.vcpkg_triplet }}-${{ steps.vars.outputs.sha_short }}
working-directory: ${{ matrix.vcpkg_path }}
#- name: Create buildenv archive
Copy link
Member

Choose a reason for hiding this comment

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

For a first step of transition it might be good to generate the old style buildenv zip.
Is it correct, that we could keep the scripts in mixxx/tools unchanged, if we leave this section active?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, currently the zip is created manually below, because export did not work in Manifest Mode.

I think this is now supported. We need to test.

@daschuer
Copy link
Member Author

I see:

Pushing NuGet to "mixxx-github-packages" failed. Use --debug for more information.
Stored binaries in 0 destinations.

This is not possible from PR. We need to configer vcpkg to not try it.

@ronso0 ronso0 changed the title Switch to mainfest mode Switch to manifest mode Aug 16, 2023
@daschuer
Copy link
Member Author

We can enable the normal export command once this reaches our repository:
microsoft/vcpkg-tool#1136

@JoergAtGithub
Copy link
Member

We can enable the normal export command once this reaches our repository: microsoft/vcpkg-tool#1136

This is merged upstream now

@JoergAtGithub
Copy link
Member

The ZIP contains another ZIP with the containing the same files again:
grafik

@JoergAtGithub
Copy link
Member

The directory/file structure in the generated .zip file looks now good!

@daschuer daschuer force-pushed the manifest_mode_ci branch 2 times, most recently from 8548231 to 1987896 Compare November 13, 2023 07:07
@daschuer daschuer mentioned this pull request Nov 15, 2023
@daschuer daschuer force-pushed the manifest_mode_ci branch 3 times, most recently from fa890ed to 6049323 Compare November 17, 2023 07:23
Caching with down and upload is slower than a fresh download
@daschuer
Copy link
Member Author

Now are all prereqesites merged and we can merge this.

During testing, I have created a script that is able to clear the packages from GitHub. You can do it with the GUI only with a security check that requires to type each package name. That is teditlitios.
What do you think should I add it to the root of this repro or to the tools folder of mixxx? Ideas?

@JoergAtGithub
Copy link
Member

Since we upload the packages from mixxxdj/vcpkg, the script to delete them should be also located here.
But I'm a bit concerned, that it' possible to upload packages from an open PR.

@daschuer
Copy link
Member Author

Into the root here?

You need to have write access to this repro to upload and delete packages. PRs only get read access. Team members also would have the force to upload packages from a PR. But they can also upload packages via any other probably more hidden route.

@JoergAtGithub
Copy link
Member

I'm not sure what deleting means for the workflow. What happens, if others build Mixxx using these packages an you run this script?

@daschuer
Copy link
Member Author

These packages are only used for caching purpose. If we delete them, they are regenerated. The mixxx repro has its own cache. We may consider in future to integrate the vcpkg run into the Mixxx repository. Than mixxx will also regenerate missing packages. But for now I prefere to keep both independent for less hassle during development.

@JoergAtGithub
Copy link
Member

I just build Mixxx 2.5 under Windows 11 with the buildenv mixxx-deps-2.5-x64-windows-6edd9a3.zip from this PR, as x64_off and x64_portable. Both compiled and run as before!
Therefore we know taht this PR doesn't break anything. Let's merge it!

@JoergAtGithub JoergAtGithub merged commit dd80a94 into mixxxdj:2.5 Nov 18, 2023
2 checks passed
@fwcd
Copy link
Member

fwcd commented Feb 5, 2024

With manifest mode, vcpkg now defaults to vcpkg_installed as the install directory 1. Still, vcpkg maintains an installed directory for metadata, which means that a default ./vcpkg install will now produce two directories:

<vcpkg root>
├─ installed
│  └─ vcpkg
│     └─ ...
└─ vcpkg_installed
   ├─ arm64-osx-release
   │  └─ ...  <-- actual build artifacts are here
   ├─ ...
   └─ vcpkg
      └─ ...

The CMake toolchain from vcpkg still defaults _VCPKG_INSTALLED_DIR to <vcpkg root>/installed, which means this check in Mixxx's CMakeLists.txt:

if(DEFINED _VCPKG_INSTALLED_DIR)
  if(NOT EXISTS "${_VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}")
    # Fail early if this part of CMAKE_PREFIX_PATH does not exist
    # else the library lookups below will fail with misleading error messages
    message(STATUS "VCPKG_TARGET_TRIPLET dir not found: ${_VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET} "
        "Make sure the VCPKG build environment is installed and contains the build for the selected triplet.")
    FATAL_ERROR_MISSING_ENV()
  else()
    message(STATUS "Using VCPKG_TARGET_TRIPLET: ${VCPKG_TARGET_TRIPLET}")
  endif()
endif()

...breaks down and a user that built the dependencies in a manual vcpkg env will get the error:

-- VCPKG_TARGET_TRIPLET dir not found: <vcpkg root>/installed/arm64-ios-release Make sure the VCPKG build environment is installed and contains the build for the selected triplet.
CMake Error at CMakeLists.txt:61 (message):
  Did you download the Mixxx build environment using
  ´<mixxx root>/tools/macos_release_buildenv.bat´ or
  ´<mixxx root>/tools/macos_buildenv.bat´(includes
  Debug)?

I see two options here:

  • Require users to set --x-install-root <vcpkg root>/installed in their ./vcpkg install invocation (the environment variable VCPKG_INSTALLED_DIR should work too)
  • Update Mixxx's CMakeLists.txt to check vcpkg_installed too

This probably only affects users who wish to build their own vcpkg environments, since the exported buildenv still contains the installed directory. Therefore, it's not a deal-breaking issue (since the user already ventured into adventurous territory by building their own dependency stack), but still potentially confusing. Perhaps we should also clarify the error message if we see a vcpkg root that's not in <mixxx root>/buildenv?

Footnotes

  1. This is likely because vcpkg encourages projects to place the vcpkg.json manifest into the project rather than the vcpkg root itself. Mixxx does not do this, for arguably valid reasons (keeping the list of dependencies in the same repo as our overlays), but we're still stepping out of "common case" territory.

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.

3 participants