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

add custom macOS triplet for Qt6 #26

Merged
merged 2 commits into from
Oct 18, 2021
Merged

add custom macOS triplet for Qt6 #26

merged 2 commits into from
Oct 18, 2021

Conversation

Be-ing
Copy link

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

No description provided.

Copy link
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.

I am afraid your renaming is not working.
I suggest to introduce another sub folder and stick with x64-osx.triplet for both.
I think exist an implicit naming convention a triplet of:
ARCHITECTURE-OS-PLATFORM.triplet

The green checkmark here is not significant, because it now falls Fack to the default x64-osx triplet

@Be-ing
Copy link
Author

Be-ing commented Oct 17, 2021

It is working fine here: mixxxdj/mixxx#4432

There is no need to reuse the x64-osx name. That would be confusing for no particular reason.

@daschuer
Copy link
Member

The triplet name x64-osx-mixxx was not working reliable after following your suggestion here:
#18 (review)

Unfortunately I cannot find the failure log anymore. I only know that the error was hard to discover and took me many ours because the error message was not mentioning the triplet file, and the error did not apperars instantly after renaming the triplet file because of caching.

Let's not repeat this.

@Be-ing
Copy link
Author

Be-ing commented Oct 17, 2021

The use of x64-osx for the custom triplet caused me some confusion working on mixxxdj/mixxx#4432. Before I set the host triplet, the Qt6 build failed because it fell back to using x64-osx which was set to a minimum target macOS 10.12 which is lower than what Qt6 supports.

Whatever trouble you ran into before is not happening in manifest mode with mixxxdj/mixxx#4432 so I don't think we need to worry about it.

@Be-ing
Copy link
Author

Be-ing commented Oct 17, 2021

the error did not apperars instantly after renaming the triplet file because of caching

This will not be an issue with manifest mode.

@daschuer
Copy link
Member

If you are sure it is working, fine. But be warned.

@daschuer
Copy link
Member

Please Update the GitHub workflow to use the new triplet name.

@Be-ing
Copy link
Author

Be-ing commented Oct 17, 2021

I have removed the GitHub Actions workflow from this repo in #29.

@daschuer
Copy link
Member

That's not yet merged. We have currently no CI for the new triplets. So please adhust the names her to have a significant test.

@Be-ing
Copy link
Author

Be-ing commented Oct 17, 2021

That is circular reasoning. We cannot merge mixxxdj/mixxx#4432 without this and you are saying to not merge this.

@Be-ing
Copy link
Author

Be-ing commented Oct 17, 2021

Please stop putting unnecessary obstacles in the way of progress.

@Be-ing
Copy link
Author

Be-ing commented Oct 17, 2021

mixxxdj/mixxx#4432 is the CI for this.

@daschuer
Copy link
Member

But it is failing.

@Be-ing
Copy link
Author

Be-ing commented Oct 17, 2021

only because Mixxx does not build with Qt6 yet

@Be-ing
Copy link
Author

Be-ing commented Oct 18, 2021

Let's merge this then I can switch the submodule in mixxxdj/mixxx#4432 from my branch to this repo.

@daschuer
Copy link
Member

You should really enable the Ci with the new triplet. It is just a one line change and 4 h waiting.
microsoft#4432 has no significant log output and no build artifacts. I think it will take a bit until it is mature.
Once the CI is done this can be merged and we can update the mixxx main with the new hash.
Lets go in small steps.

@Be-ing Be-ing merged commit 1b70fe2 into mixxxdj:2.4 Oct 18, 2021
@Be-ing
Copy link
Author

Be-ing commented Oct 18, 2021

This bikeshedding helps nobody.

@Be-ing Be-ing deleted the osx_triplets branch October 18, 2021 01:51
@Be-ing
Copy link
Author

Be-ing commented Oct 18, 2021

Running CI on this repo is utterly pointless and a waste of time and electricity.

no significant log output and no build artifacts

This is not true. Just because you were not looking at them does not mean they do not exist. I have been spending days looking at the log outputs.

@Be-ing
Copy link
Author

Be-ing commented Oct 18, 2021

I think it will take a bit until it is mature.

This was incredibly infuriating to read. I have already taken enough insults about the quality of my work (that other people aren't doing). I have spent days toiling over GitHub Actions logs, sometimes waiting over 4 hours to find out the effects of my changes. It works. I know it works. Just because you weren't paying attention doesn't mean I wasn't.

@Holzhaus
Copy link
Member

@Be-ing I think we have a concensus that it's not okay to merge your own PR. Especially when there is no approval and a reviewer explicitly requested changes.

If you think that the discussion goes in circles and is not productive anymore, you may call for a vote and we can decide as a team how to continue. If you consider this approach insufficient, we can also check if we can find a better governance model by discussing it on Zulip. But ignoring the opinion of other devs without a majority decision is not okay.

@Be-ing
Copy link
Author

Be-ing commented Oct 18, 2021

you may call for a vote and we can decide as a team how to continue

This requires other people to pay attention and speak up, not let one person get their way just by being willing to repeat themselves more than anyone cares to argue with.

@Holzhaus
Copy link
Member

Holzhaus commented Oct 18, 2021

This requires other people to pay attention and speak up, not let one person get their way just by being willing to repeat themselves more than anyone cares to argue with.

You did not even try to get a majority decision. By the time you merged it, the PR was less than 1 1/2 days old. That is not much time for a volunteer project.

Letting one person get their way by being willing to ignore our decision making process and abuse commit privileges isn't any better. Please don't do this.

@Be-ing
Copy link
Author

Be-ing commented Oct 18, 2021

Sorry. I got extremely frustrated by @daschuer insulting my work. I did nothing but work on this for several days straight, and now I get insulted for it?!??

daschuer pushed a commit to daschuer/vcpkg that referenced this pull request Apr 7, 2022
* [cairomm] update to 1.16.1 (microsoft#23903)

Cairo:
* Surface::Type: Deprecate WIN32, add WIN32_SURFACE
  (Kjell Ahlstedt) Issue mixxxdj#26, merge request !14

Build:
* cairommconfig.h.*: Don't dllimport on MinGW
  (Chun-wei Fan) Merge reqest !16
  (Chun-wei Fan) Issue gtkmm#90 (Lukas K.)
* Meson build: Make it possible to use cairomm as a subproject
  (Kjell Ahlstedt)
* Meson build: No implicit_include_directories
  (Kjell Ahlstedt)
* MSVC build: exception.h: Export Cairo::logic_error selectively
  (Chun-wei Fan) Merge request !17

* [cairomm] remove patch (microsoft#23903)

The WIN32 constant has been renamed to WIN32_SURFACE , and the
WIN32 -> WIN32_SURFACE alias is only provided for non-win32
sytems, therefore our previous patch is no longer needed.

* [cairomm] Support MSVC2022 (microsoft#23903)

Manually add ed1ce9a630b375b0f43435e34fbe690eb8276178 from upstream,
which prevents MSVC 2022 toolchains from being overridden by the
meson port file, resulting in corrupted binarycache metadata

* [cairomm] update version registries (microsoft#23903)

* [cairomm] add license (microsoft#23903)

* [cairomm] regenerate license registry (microsoft#23903)

Co-authored-by: Schaich <[email protected]>
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