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

Backport #2859 to release/2.1.x #2890

Merged
merged 11 commits into from
Feb 23, 2025
Merged

Conversation

antonfirsov
Copy link
Member

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

Backport #2859 to release/2.1.x

@antonfirsov
Copy link
Member Author

Please squash this mess if it will ever build 😅

I've seen some unrelated WEBP test failures locally, probably they will show up here.

@antonfirsov
Copy link
Member Author

Even BMP failures. I don't understand this, neither we changed the product code nor we added tests since 2.1.9. Will continue investigating tomorrow, but if anyone has an idea, let me know!

@JimBobSquarePants
Copy link
Member

@antonfirsov Github keeps changing what’s installed on images. I discovered with V3 branch that weirdly you had to install NET 8 along with the current target version for anything to build. I would use the yaml file from that branch as the basis and change the target frameworks.

@brianpopow
Copy link
Collaborator

I cannot reproduce the bmp test failures on windows. When I run:

dotnet test -c Release --filter "Format=Bmp"

All tests are green.

Comment on lines 614 to 617
if (TestEnvironment.IsLinux)
{
throw new SkipTestException("See discussion on https://github.com/SixLabors/ImageSharp/pull/2890");
}
Copy link
Member Author

Choose a reason for hiding this comment

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

The Linux failure is a mystery: it only shows when running the test suite. If I run a console app against the compiled ImageSharp.dll on Ubuntu, the image loads without an issue. I have spent an hour trying to hook a debugger into VS Code, but it keeps ignoring my breakpoints.

I ended up skipping the test on Linux. (1) It's likely a false alert that appears only with dotnet test (2) likely not a regression against the previous version, (3) an edge-case anyways.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is ok to skip those bmp tests on linux, they are extremely niche bitmap variants. It's also a mystery to me why this is different on linux than on windows.

Copy link
Member Author

@antonfirsov antonfirsov Feb 21, 2025

Choose a reason for hiding this comment

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

It was an LFS issue (╯°□°)╯︵ ┻━┻

I had to add capital-case *.BMP to the relevant gitattributes list to make Linux happy. No idea why don't we have it on other branches.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wow, good find

Comment on lines +77 to +86
- name: Install Ubuntu prerequisites
if: ${{ contains(matrix.options.os, 'ubuntu') }}
run: |
# libssl 1.1 (required by old .NET runtimes)
wget http://archive.ubuntu.com/ubuntu/pool/main/o/openssl/libssl1.1_1.1.0g-2ubuntu4_amd64.deb
sudo dpkg -i libssl1.1_1.1.0g-2ubuntu4_amd64.deb

# libgdiplus
sudo apt-get -y install libgdiplus libgif-dev libglib2.0-dev libcairo2-dev libtiff-dev libexif-dev

Copy link
Member Author

Choose a reason for hiding this comment

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

I would use the yaml file from that branch as the basis and change the target frameworks.

I decided to keep the current state as the basis to avoid accidentally changing the package targets.

The magic to revive old .NET versions is to manually install libssl 1.1.

@antonfirsov
Copy link
Member Author

@JimBobSquarePants it should be good to merge this now and start publishing stuff.

Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

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

LGTM Thanks! 👍

@JimBobSquarePants JimBobSquarePants merged commit 627b5f7 into release/2.1.x Feb 23, 2025
14 checks passed
@JimBobSquarePants JimBobSquarePants deleted the af/backport-2859 branch February 23, 2025 23:31
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