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

remove GitHub Actions workflow for vcpkg archive #29

Closed
wants to merge 1 commit into from

Conversation

Be-ing
Copy link

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

Replaced by using vcpkg in manifest mode
mixxxdj/mixxx#4432

@daschuer
Copy link
Member

This should not be merged before mixxxdj/mixxx#4432

Are we sure every windows/macOs user is able to setup GitHub package cache?
Mybe we should wait at least until the new workflow is settled.

@Be-ing
Copy link
Author

Be-ing commented Oct 17, 2021

It does not really matter whether this or mixxxdj/mixxx#4432 is merged first.

@daschuer
Copy link
Member

Can we close this? With out a CI future changed with this are a blind flight. No contributor has all target system to verify changes.

I can imagine to upload single packages to Assure if mixxxdj/mixxx#4432 had been established.

@Be-ing
Copy link
Author

Be-ing commented Nov 14, 2021

No this is just a waste of electricity.

@daschuer
Copy link
Member

Please explain.

@Be-ing
Copy link
Author

Be-ing commented Nov 14, 2021

The workflow in manifest mode is to checkout the submodule to a new commit and make a PR for https://github.com/mixxxdj/mixxx. Running CI on this repo is pointless and very slow.

@daschuer
Copy link
Member

This extra step will slow down the development process significant.
We can move the manifest file to this repro and upload the results to Assure. This gives immediately results and saves energy, because of cache hits in the mixxxdj/mixxx repro.

@Be-ing
Copy link
Author

Be-ing commented Nov 17, 2021

No, this makes the development process significantly easier. Manifest mode is usable for working on vcpkg packaging locally, this workflow file that only runs on CI is not. Also, just because this workflow finishes successfully, that is not a guarantee that the build will actually work with Mixxx. Building dependencies with manifest mode together with the application checks that everything works together.

We can move the manifest file to this repro

As I've already said, I tried this and it does not work. vcpkg is designed to be used as a submodule in an application with a vcpkg.json in the root of the application's repository. Please stop fighting upstreams' designs, this benefits nobody.

@daschuer
Copy link
Member

This assumes that you have all targets locally, which is not the case.

Also, just because this workflow finishes successfully, that is not a guarantee that the build will actually work with Mixxx.

Of cause not. It is the other way around if you have a working local copy that fails here, it is a guarantee that it fails also when build with Mixxx.

Building dependencies with manifest mode together with the application checks that everything works together.

This is out of question and a required test.
The issue is just that we need the merge commit here fist for the final commit hash.

I tried this and it does not work.

What exactly did not work?

I think that it will finally a good solution here:

  • Maintain here changes from upstream
  • Create here the azure packages for all our targets
  • Provide them as binary block to allow building without of the hassle to set up manifest mode.
  • If this is working. Create a PR on mixxxdj/mixxx with the updated manifest.
  • Do smoke tests on all targets
  • Merge.

The other way around you have a commit hash in the PR, that will not match the final commit hash here.

That is one reason by the way why I recommend NOT to use a strong comit hash reference!

We don't have a commit hash dependency, just because we can't have it on Linux. Mixxx can always build with different versions of dependent packages.

@daschuer
Copy link
Member

By the way, I am currently working on creating the arm64-osx packages via crosscomiling via the GitHub workflows here: https://github.com/daschuer/vcpkg/tree/arm64-osx-min11.0

@Be-ing
Copy link
Author

Be-ing commented Nov 18, 2021

The vcpkg.json file is designed to be in the application repository. The manifest file is not intended to be changed frequently. This is also how every other language-specific package manager such as Cargo and NPM work. vcpkg is designed so that application developers write a vcpkg.json file, add vcpkg as a submodule, then simply merge updates of vcpkg periodically. Forking the vcpkg repository is not intended to be a requirement (it is for us because we use custom triplets to set the minimum macOS version to build with). I think it is inappropriate to debate this; if you have a problem with it you can take that upstream. I am sure they will tell you it is how it is on purpose.

The workflow with manifest mode is:

  1. Make change to vcpkg repo and open PR
  2. Open PR on mixxx repo switching the vcpkg submodule to the PR
  3. If the mixxx PR works, merge the vcpkg PR
  4. Switch the vcpkg submodule to the branch of the mixxxdj/vcpkg repo
  5. Merge mixxx PR

@daschuer
Copy link
Member

I don't think that this is compared to other package manager that are maintained from a third party, because the package maintainers are responsible to verify the quality, including the binary builds.

In case of vcpkg, no verified binary builds are distributed. It is up to us to do that. So it sounds reasonable to me to split these task into to CI runs.

  • Build packages here
  • Build Mixxx in mixxxdj/mixxx

I tried this and it does not work.

What exactly did not work?

You have not anwered the question. What is the reason it can't work?

@daschuer
Copy link
Member

The proposed does indeed work, but the issue is that no other can verify it without building the package at home. And we have the extra commit that needs to be done to retarget to the different vcpkg branches.

This will be not the case if we upload the packages here.

@Be-ing
Copy link
Author

Be-ing commented Nov 19, 2021

What is the reason it can't work?

As I have said many times, vcpkg archive is incompatible with manifest mode.

It turns out there is an undocumented command line parameter for vcpkg to override its default behavior of looking for the vcpkg.json file in the directory above the exectuable. Using that on CI in this repo would be a bad idea though. First, it is an undocumented hack and basing our infrastructure on undocumented hacks is a setup for trouble. Moreover, it would require duplicating the vcpkg.json file into this repository so it would have to be kept in sync with the mixxx repository manually, adding to maintenance burden. This could be overcome with a very ugly hack of cloning the mixxx repo on CI in this repo, at which point we may as well just run CI on the mixxx repo as vcpkg is designed.

@Be-ing
Copy link
Author

Be-ing commented Nov 19, 2021

I don't think there's any point discussing this further and I think it is inappropriate to challenge upstream's design decisions here. It is time to vote whether to merge this. React with 👍 to this comment to merge or 👎 to not merge.

Pros for merging this:

  • The artifacts created by this GitHub Actions workflow are no longer used with vcpkg manifest mode mixxx#4432, so getting rid of this reduces maintenance burden.
  • Running CI on the mixxx repo instead not only verifies that the packages build, but also that Mixxx builds with the packages.

Cons for merging this:

  • Testing changes to vcpkg requires updating the vcpkg submodule in the mixxx repository.

Pros for maintaining this GitHub Actions workflow:

  • Changes to vcpkg can be tested if they build before making a pull request for the mixxx repository.

Cons for maintaining this GitHub Actions workflow:

  • CI still needs to be run on the mixxx repository regardless of this repository and that tests whether packages build.
  • vcpkg archive does not work with vcpkg.json manifest files.
  • Requires maintaining code that is not used in the build process of Mixxx.
  • Requires maintaining code that is not usable locally and only runs on CI.
  • This workflow uploads giant ZIP (> 1 GB) archives to https://downloads.mixxx.org which are not used anymore so they waste space on the server and require periodic cleanup.
  • Caching is flaky with this workflow. If building one package fails, none of them are cached. Moreover the GitHub Actions cache expires after 30 days and we often don't update dependencies for more than 30 days.
  • Running CI on this repo is very slow, taking up to 4 hours, and a waste of energy.

@ywwg
Copy link
Member

ywwg commented Nov 19, 2021

I don't really understand the issues in play here so I don't feel qualified to vote. My sense is that although submodules are annoying and cumbersome, that's how git works and it's a valid workflow. If that's how vcpkg is designed to work, then we should do it that way. (That's more of an argument for 4432)

It would seem not to make sense to do this cleanup step before we have migrated over to the submodule solution in the other PR. Of the reasons you say you want to do this now, most of them do not seem urgent -- disk space and wasted CPU. The one that is compelling is "If building one package fails, none of them are cached". Are you saying that if vcpkg fails to build, mixxx isn't cached? How often does that happen? Can you be more concrete about the burden of keeping this active to justify blowing it away sooner?

@Be-ing
Copy link
Author

Be-ing commented Nov 19, 2021

The only reason this is urgent is because @daschuer is insisting on blocking the PR (#33) needed for mixxxdj/mixxx#4432 on whether the CI on this repository continues to succeed, even though that is pointless because the CI on this repo is not used with mixxxdj/mixxx#4432, thereby creating a circular dependency.

I would be fine with merging #33 then merging this later; it really doesn't matter except that @daschuer is gatekeeping #33 and thereby mixxxdj/mixxx#4432 on this.

@Be-ing
Copy link
Author

Be-ing commented Nov 19, 2021

The one that is compelling is "If building one package fails, none of them are cached". Are you saying that if vcpkg fails to build, mixxx isn't cached? How often does that happen? Can you be more concrete about the burden of keeping this active to justify blowing it away sooner?

I mean that using GitHub Actions on this repository to test if packages build is awfully cumbersome. If any of the packages fail to build, none of the ones that did succeed get cached, which is a huge pain for 4 hour build process.

Using vcpkg in manifest mode and vcpkg's binary caching feature as mixxxdj/mixxx#4432 does, this is not a problem because vcpkg uploads each package to GitHub Packages as it is built instead of relying on GitHub Actions' cache.

@daschuer
Copy link
Member

I have just tried to put a vcpkg.json containing a manifest in the root of this repository.
And against your claims it works out of the box with a plain
./vcpkg install --overlay-ports overlay/ports
No hacks, it just creates a "vcpkg_installed" directory with the same content like in the "installed" directory.

Let's reiterate our discussion:

We can move the manifest file to this repro

I tried this and it does not work.

What exactly did not work?

???

You have not answered the question. What is the reason it can't work?

As I've already said, I tried this and it does not work.

An than your long, IMHO inappropriate answer here: #29 (comment)

My best guess is that you have not even tried my proposal, else it would be really unfair to use wrong arguments knowingly. Instead you use an offensive, annoyed voice: "As I have said many times ..." following by wrong statements with keywords that made my ideas look in a bad light:
"undocumented" "override its default" "a bad idea" "undocumented hack" "setup for trouble" "duplicated vcpkg.json file" "adding to maintenance burden" "very ugly hack"
Than you ask for a vote. Is this fair?

I think you have failed to ask for such an all or nothing vote recently for similar reasons. A vote can only be a last resort if all pro and cons have been exchanged in a fair manner, and there is no compromise left. This seems not yet the case here.

@Be-ing
Copy link
Author

Be-ing commented Nov 19, 2021

My best guess is that you have not even tried my proposal

This is a false assumption.

./vcpkg install --overlay-ports overlay/ports

You're ignoring everything else that would be required, which would be duplicated code from the mixxx repository.

@Be-ing
Copy link
Author

Be-ing commented Nov 19, 2021

Is this fair?

Is it fair to gatekeep merging fully functioning code on irrelevant non-issues?

@daschuer
Copy link
Member

You're ignoring everything else that would be required, which would be duplicated code from the mixxx repository.

I don't see any duplication, because this repo will be checked out into the Mixxx build tree, right?

Please outline your ideas. What exactly else is required?

@Be-ing
Copy link
Author

Be-ing commented Nov 19, 2021

I don't see any duplication, because this repo will be checked out into the Mixxx build tree, right?

Huh? This is what mixxxdj/mixxx#4432 already does in the Mixxx repository.

@Be-ing
Copy link
Author

Be-ing commented Nov 19, 2021

Please outline your ideas.

I already have. It is this PR, #33, and mixxxdj/mixxx#4432. Asking me to repeat myself is just gatekeeping.

@daschuer
Copy link
Member

Cons for merging this and removing the CI

  • We no longer have a green check mark in Pull requests

Cons for maintaining this GitHub Actions workflow:

  • CI still needs to be run on the mixxx repository regardless of this repository and that tests whether packages build.
    • That needs to be done anyway. If have uploaded the packages here, the Mixxx CI is kept fast.
  • vcpkg archive does not work with vcpkg.json manifest files.
    • that is simply wrong
  • Requires maintaining code that is not used in the build process of Mixxx.
    • Why do you think that?
  • Requires maintaining code that is not usable locally and only runs on CI.
    • Why is that an issue? The build.yml file of a github workflow runs only on GitHub
  • This workflow uploads giant ZIP (> 1 GB) archives to https://downloads.mixxx.org which are not used anymore so they waste space on the server and require periodic cleanup.
    • The question if we want to do the bundle upload or not is kind of unrelated here, we can change our opinion at any time. For now i like to maintain the package, because using it you get around all caching issues and API key issues and the commit hash dependency issue.
  • Caching is flaky with this workflow. If building one package fails, none of them are cached. Moreover the GitHub Actions cache expires after 30 days and we often don't update dependencies for more than 30 days.
    • That's why we use Azzure, right? So this argument does no longer apply.
  • Running CI on this repo is very slow, taking up to 4 hours, and a waste of energy.
    • This argument does also not apply. It does not matter where the package is build as long we use a common cache, which is the case "After" it is merged. Remember you do not have the right to upload from your private Repro.

@daschuer
Copy link
Member

You're ignoring everything else that would be required, which would be duplicated code from the mixxx repository.

Please outline your ideas. What exactly else is required?

I already have.

Can you point me to a post? I cannot find, where you have written what else will be required, other than putting the vcpkg.json in the root of this repro and adjust the workflow to use it and use Assure for aching.

@Be-ing
Copy link
Author

Be-ing commented Nov 20, 2021

vcpkg archive does not work with vcpkg.json manifest files.

that is simply wrong

I can only assume you did not actually try this.

mixxx on  vcpkg_manifest [$!?⇕] via △ v3.21.3 via 🐍 v3.10.0 
❯ vcpkg/vcpkg export
vcpkg export does not support manifest mode, in order to allow for future design considerations. You may use export in classic mode by running vcpkg outside of a manifest-based project.
Note: Updating vcpkg by rerunning bootstrap-vcpkg may resolve this failure.

@Be-ing
Copy link
Author

Be-ing commented Nov 20, 2021

I am not wasting my time bikeshedding this for weeks. Vote. If you are opposed to this PR, then you can vote against it. If you boycott the process, your voice doesn't count.

@daschuer
Copy link
Member

With manifest mode we don't need "vcpkg/vcpkg export" anymore. I think it is sufficient to zip the vcpk_installed folder.

@Be-ing
Copy link
Author

Be-ing commented Nov 20, 2021

Once again, you are posting false information that you could trivially verify is false on your own.

@Be-ing
Copy link
Author

Be-ing commented Nov 20, 2021

If anyone else has questions I can clarify before voting, you are welcome to ask. But demanding weeks of discussion justifying every simple change is abusive. You don't get to have your way just because you're willing to spend more time arguing than anyone else.

@daschuer
Copy link
Member

I have just made a div and the "vcpk_installed" folder is exactly the same as the "installed" folder, so the info was not false at all. The CMAKE_TOOLCHAIN_FILE part in the "script" folder is missing though, but that is not much of an issue. We may consider different ways to get around the api key issue, with this folder alone.

@daschuer
Copy link
Member

Here is a [POC] of my idea #34
please verify.

@Be-ing Be-ing mentioned this pull request Nov 20, 2021
@Be-ing
Copy link
Author

Be-ing commented Nov 20, 2021

I have just made a div and the "vcpk_installed" folder is exactly the same as the "installed" folder, so the info was not false at all. The CMAKE_TOOLCHAIN_FILE part in the "script" folder is missing though, but that is not much of an issue. We may consider different ways to get around the api key issue, with this folder alone.

Are you serious?

@daschuer
Copy link
Member

Superseded by #80

@daschuer daschuer closed this Nov 18, 2023
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