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

dev-games/aseprite: add 1.3.5 #35619

Closed
wants to merge 2 commits into from

Conversation

winterheart
Copy link
Contributor

Add new 1.3.5

@gentoo-bot
Copy link

Pull Request assignment

Submitter: @winterheart
Areas affected: ebuilds
Packages affected: dev-games/aseprite

dev-games/aseprite: @winterheart, @gentoo/proxy-maint

Linked bugs

Bugs linked: 924692


In order to force reassignment and/or bug reference scan, please append [please reassign] to the pull request title.

Docs: Code of ConductCopyright policy (expl.) ● DevmanualGitHub PRsProxy-maint guide

@gentoo-bot gentoo-bot added self-maintained The PR changes only packages that are maintained by the submitter (i.e. no need to ask anybody else) assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR. labels Mar 4, 2024
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2024-03-04 14:02 UTC
Newest commit scanned: 87c2788
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/e3f26e8f92/output.html

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2024-04-15 09:26 UTC
Newest commit scanned: 364c3b1
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/717ac0875e/output.html

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2024-04-15 11:19 UTC
Newest commit scanned: ca1f9ca
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/4f462df5c6/output.html

Copy link
Member

@juippis juippis left a comment

Choose a reason for hiding this comment

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

Sorry for the delay and thanks!

@eli-schwartz
Copy link
Contributor

@juippis,

This PR reverts QA changes and shouldn't have been merged.

# that is disabled, fails again with ODR and lto-type-mismatch issues.
#
# There are a lot of issues, so don't trust any fixes without thorough
# testing.
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this tested? I think not. At minimum, you haven't patched the QA violation in the skia archive, nor have you upgraded the skia revision.

@juippis
Copy link
Member

juippis commented Apr 16, 2024

This PR didn't revert anything, it added a new version with a fix hopefully. If the proposed fix doesn't work, I guess the old workaround can be re-applied.

@eli-schwartz
Copy link
Contributor

The proposed fix fails to work, because the proposed fix solves strict-aliasing issues but not ODR issues.

Updating to a new version and removing a QA fix certainly appears to be reverting a fix. But @winterheart says I'm not allowed to add QA fixes to ebuilds after several months of an open ticket, because it is a "violation of maintainer autonomy", which looks like it has something to do with the removal of the QA fix.

I cannot possibly disagree more.

@juippis
Copy link
Member

juippis commented Apr 16, 2024

Okay, I see there's some history here. I don't look at git log category/package before checking these bumps. The filter-lto could've potentially been there for years (via filter-flags, and no, didn't look at diff for deleted file with a bug link). What parameters you want me to test the 1.3.5 with? -Werror=strict-aliasing -Werror=odr -Werror=lto-type-mismatch? I'll add the flags back if it fails.

My POV:

  • Gentoo maintainer is aware of the issue (upstream PR link by the Gentoo maintainer),
  • upstream has an issue filed,
  • upstream has a PR to fix the problem.

@winterheart is a long-standing contributor with good reputation, so obviously I'm willing to trust they've tested the patch. We often end up carrying these "workarounds" (like redundant seds) in ebuilds, so it's nice to see an attempt cleaning them out.

And to @winterheart : I admit it would've been nice to see a PR with you tagged to review it, but the truth also is that some things just need to be pushed or otherwise they'd end up rotting/waiting for different maintainers. In my opinion the filter-lto is a rather simple commit, and the bug was open long enough to react. So I really don't see that much harm in it.

But since upstream is aware of the issue, that's THE best place to fix it ultimately. Let's work with each other, rather than against each other here?

@thesamesam
Copy link
Member

Yes, certain work falls under broad-scope QA. In this case, it's about damage limitation (reducing risk of miscompilation). If maintainers want to do further work to fix things properly, they can, although it might be a bit annoying sometimes if it blocks the LTO tracker when it's not strictly a blocker, even though it's related.

It's a shame that the additional Skia issue was missed, which Eli noted both on the bug and I would've expected @winterheart to spot when testing themselves, but mistakes happen.

@juippis
Copy link
Member

juippis commented Apr 16, 2024

So looks like with the patch append-flags -fno-strict-aliasing can be dropped, but filter-lto must be put back so -flto -Werror=strict-aliasing -Werror=odr -Werror=lto-type-mismatch will compile.

@eli-schwartz do you want me to author you, or should I just push it? And do you agree with this?

@eli-schwartz
Copy link
Contributor

Yes, the patch should allow dropping -fno-strict-aliasing. I haven't tested it myself but I'm more than happy to take that on faith.

I know that -flto -Werror=strict-aliasing -Werror=odr -Werror=lto-type-mismatch will fail due to odr and lto-type-mismatch since I only added it a couple days ago and because I tested it with the upstream 1.3.5 and 1.3.6 releases (but didn't submit the updated versions, specifically to avoid stepping on the toes of the active maintainer -- I'm acting as an agent of QA here, and that begins and ends with LTO fixes).

Even 1.3.6 has the LTO errors upstream, and the patch doesn't include LTO fixes, only strict-aliasing fixes.

@eli-schwartz do you want me to author you, or should I just push it? And do you agree with this?

I don't mind one way or another who the author is. If the comment (after tweaking to adapt to strict-aliasing being obsolete) is substantially the same as mine, I suppose a reasonable argument can be made for Co-authored-by?

And yes -- re-adding the filter-lto on its own sounds correct.

gentoo-bot pushed a commit that referenced this pull request Apr 16, 2024
 - originally made in 95f2613.

Closes: #35619
Co-authored-by: Eli Schwartz <[email protected]>
Signed-off-by: Joonas Niilola <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR. self-maintained The PR changes only packages that are maintained by the submitter (i.e. no need to ask anybody else)
Projects
None yet
7 participants