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.

Sign up for GitHub

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

Jump to bottom

[gexiv2] new port #43600

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[gexiv2] new port #43600

wants to merge 1 commit into from

Conversation

talregev
Copy link
Contributor

@talregev talregev commented Feb 2, 2025

  • Changes comply with the maintainer guide.
  • The name of the port matches an existing name for this component on https://repology.org/ if possible, and/or is strongly associated with that component on search engines.
  • Optional dependencies are resolved in exactly one way. For example, if the component is built with CMake, all find_package calls are REQUIRED, are satisfied by vcpkg.json's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx.
  • The versioning scheme in vcpkg.json matches what upstream says.
  • The license declaration in vcpkg.json matches what upstream says.
  • The installed as the "copyright" file matches what upstream says.
  • The source code of the component installed comes from an authoritative source.
  • The generated "usage text" is accurate. See adding-usage for context.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is in the new port's versions file.
  • Only one version is added to each modified port's versions file.

@talregev talregev force-pushed the TalR/gexiv2 branch 2 times, most recently from 79c98d8 to 3be0379 Compare February 2, 2025 23:19
@talregev talregev marked this pull request as ready for review February 2, 2025 23:28
@talregev talregev force-pushed the TalR/gexiv2 branch 3 times, most recently from e314d3d to 5393705 Compare February 4, 2025 13:40
dg0yt
dg0yt reviewed Feb 4, 2025
View reviewed changes
ports/gexiv2/vcpkg.json Outdated
"description": "A GObject-based Exiv2 wrapper.",
"homepage": "https://gitlab.gnome.org/GNOME/gexiv2/",
"license": "GPL-2.0-or-later",
"supports": "!windows | mingw",
Copy link
Contributor

Choose a reason for hiding this comment

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

Really no windows support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I tried and it not compile the lib file. I am not sure why.

Copy link
Contributor

Choose a reason for hiding this comment

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

supports must be consistent with the upstream. The upstream supports windows and mingw. If there are triplets that cannot be compiled, please write to ci.baseline.txt. refer to: #35928.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added on ci.baseline.txt

Copy link
Contributor

Choose a reason for hiding this comment

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

I was able to grab the x64-windows logs before CI was canceled. It built no link lib for the DLL. Well, it simply doesn't seem to support DLLs on windows. Unfortunately I don't know the build result for static linkage.

In addition, I think that ci.baseline.txt is wrong for ports which don't support a target at all. We had this with octave: Wasting several hours CI time per week while absolute not supporting MSVC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MonicaLiu0311 upstream state that they support dll on windows, but they lack dllexport/dllimport markup, or .def files.
This is consider a bug from upstream:
https://gitlab.gnome.org/GNOME/gexiv2/-/issues/85#note_2345982

With other words, if we do not support it, it's most likely a bug (wherever, might be even in meson), not intent

Please review it again. Thank you.

Copy link

Choose a reason for hiding this comment

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

As a GIMP developer on Windows I can confirm that gexiv2 supports DLL's on Windows at least when built with gcc or clang supplied by the MSYS2 project.

Sure, but in absence of dllexport/dllimport markup, or .def files, there are no usable link libraries for DLLs with MSVC.

Right. That weirdness. Blast from the past.

Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC this is the confirmation that the package doesn't support DLLs support with MSVC.

If yes, this PR should be adjusted to build static libs for VCPKG_TARGET_IS_WINDOWS AND NOT VCPKG_TARGET_IS_MINGW. Unless there are more pitfalls for the MSVC toolchain.

Copy link

@phako phako Feb 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

There is a merge request where I try to add support, but Windows honestly is putting up quite some fight getting a compile environment up and running. Also, FOSS isn't a one-way-road, you know. Patches to make things work for your platform are welcome.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, FOSS isn't a one-way-road, you know. Patches to make things work for your platform are welcome.

Also, your are talking to voluntary contributors which do upstream patches as appropriate. Some of us only using MSVC in vcpkg CI.

ports/gexiv2/portfile.cmake Show resolved Hide resolved
@MonicaLiu0311 MonicaLiu0311 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Feb 5, 2025
@talregev
Copy link
Contributor Author

talregev commented Feb 8, 2025 •
edited
Loading

@LilyWangL can you change assing to available reviewer?

@MonicaLiu0311 MonicaLiu0311 marked this pull request as draft February 8, 2025 10:19
@talregev talregev force-pushed the TalR/gexiv2 branch 2 times, most recently from a38f50f to 814dfdc Compare February 8, 2025 17:42
@talregev talregev marked this pull request as ready for review February 8, 2025 17:42
@talregev
Copy link
Contributor Author

talregev commented Feb 9, 2025

@MonicaLiu0311 I answer you review comments.
Please review again. Thank you.

@phako
Copy link

phako commented Feb 10, 2025

Hi, upstream here. Can you tell me how I test this myself on a windows 11 machine?

@talregev
Copy link
Contributor Author

talregev commented Feb 10, 2025 •
edited
Loading

Hi, upstream here. Can you tell me how I test this myself on a windows 11 machine?

Hi, I am not the maintainer of vcpkg, and my answer is not official.
the easiest way to test it, is under vcpkg.
Make sure you already install git and visual studio on your windows 11.
You can use the free visual studio 2022 community.
You can also find more detail on readme.
Clone my repo with my branch:

git clone https://github.com/talregev/vcpkg -b TalR/gexiv2
cd vcpkg
bootstrap-vcpkg.bat
vcpkg install gexiv2

I also send you logs:
gexiv2.zip

@talregev
Copy link
Contributor Author

@MonicaLiu0311 Please comment and tell me your review / changes that need to be made for this PR.

@MonicaLiu0311
Copy link
Contributor

Since upstream is already adding the .def file, please wait a few days and try again to build the windows dll:
https://gitlab.gnome.org/GNOME/gexiv2/-/merge_requests/86.

@talregev
Copy link
Contributor Author

talregev commented Feb 12, 2025 •
edited
Loading

Since upstream is already adding the .def file, please wait a few days and try again to build the windows dll: https://gitlab.gnome.org/GNOME/gexiv2/-/merge_requests/86.

Are you expect me to add it here as patch?

@phako
Copy link

phako commented Feb 12, 2025

It would be awesome if you could test it on my behalf because I cannot get it running on my machine.

@talregev
Copy link
Contributor Author

It would be awesome if you could test it on my behalf because I cannot get it running on my machine.

I tested, and it give me an error. I will also share the log:

[14/15] "C:/Program Files/Microsoft Visual Studio/2022/Community/VC/Tools/MSVC/14.42.34433/bin/Hostx64/x64/link.exe"  /MACHINE:x64 /OUT:gexiv2/gexiv2-2.dll gexiv2/gexiv2-2.dll.p/meson-generated_.._gexiv2-enums.cpp.obj gexiv2/gexiv2-2.dll.p/gexiv2-metadata.cpp.obj gexiv2/gexiv2-2.dll.p/gexiv2-metadata-exif.cpp.obj gexiv2/gexiv2-2.dll.p/gexiv2-metadata-gps.cpp.obj gexiv2/gexiv2-2.dll.p/gexiv2-metadata-iptc.cpp.obj gexiv2/gexiv2-2.dll.p/gexiv2-metadata-xmp.cpp.obj gexiv2/gexiv2-2.dll.p/gexiv2-stream-io.cpp.obj gexiv2/gexiv2-2.dll.p/gexiv2-preview-properties.cpp.obj gexiv2/gexiv2-2.dll.p/gexiv2-preview-image.cpp.obj gexiv2/gexiv2-2.dll.p/gexiv2-log.cpp.obj gexiv2/gexiv2-2.dll.p/gexiv2-startup.cpp.obj "-INCREMENTAL" "/release" "/nologo" "/DEBUG" "/PDB:gexiv2\gexiv2-2.pdb" "/DLL" "/DEF:..\src\gexiv2-0-fc7db9bfe0.clean\gexiv2\gexiv2.def" "/IMPLIB:gexiv2\gexiv2.lib" "-machine:x64" "-nologo" "-debug" "/LIBPATH:C:/src/vcpkg/installed/x64-windows/debug/lib" "C:/src/vcpkg/installed/x64-windows/debug/lib/gobject-2.0.lib" "C:/src/vcpkg/installed/x64-windows/debug/lib/glib-2.0.lib" "C:/src/vcpkg/installed/x64-windows/debug/lib/intl.lib" "C:/src/vcpkg/installed/x64-windows/debug/lib/exiv2.lib" "C:/src/vcpkg/installed/x64-windows/debug/lib/gio-2.0.lib" "kernel32.lib" "user32.lib" "gdi32.lib" "winspool.lib" "shell32.lib" "ole32.lib" "oleaut32.lib" "uuid.lib" "comdlg32.lib" "advapi32.lib"
FAILED: gexiv2/gexiv2-2.dll gexiv2/gexiv2-2.pdb 
"C:/Program Files/Microsoft Visual Studio/2022/Community/VC/Tools/MSVC/14.42.34433/bin/Hostx64/x64/link.exe"  /MACHINE:x64 /OUT:gexiv2/gexiv2-2.dll gexiv2/gexiv2-2.dll.p/meson-generated_.._gexiv2-enums.cpp.obj gexiv2/gexiv2-2.dll.p/gexiv2-metadata.cpp.obj gexiv2/gexiv2-2.dll.p/gexiv2-metadata-exif.cpp.obj gexiv2/gexiv2-2.dll.p/gexiv2-metadata-gps.cpp.obj gexiv2/gexiv2-2.dll.p/gexiv2-metadata-iptc.cpp.obj gexiv2/gexiv2-2.dll.p/gexiv2-metadata-xmp.cpp.obj gexiv2/gexiv2-2.dll.p/gexiv2-stream-io.cpp.obj gexiv2/gexiv2-2.dll.p/gexiv2-preview-properties.cpp.obj gexiv2/gexiv2-2.dll.p/gexiv2-preview-image.cpp.obj gexiv2/gexiv2-2.dll.p/gexiv2-log.cpp.obj gexiv2/gexiv2-2.dll.p/gexiv2-startup.cpp.obj "-INCREMENTAL" "/release" "/nologo" "/DEBUG" "/PDB:gexiv2\gexiv2-2.pdb" "/DLL" "/DEF:..\src\gexiv2-0-fc7db9bfe0.clean\gexiv2\gexiv2.def" "/IMPLIB:gexiv2\gexiv2.lib" "-machine:x64" "-nologo" "-debug" "/LIBPATH:C:/src/vcpkg/installed/x64-windows/debug/lib" "C:/src/vcpkg/installed/x64-windows/debug/lib/gobject-2.0.lib" "C:/src/vcpkg/installed/x64-windows/debug/lib/glib-2.0.lib" "C:/src/vcpkg/installed/x64-windows/debug/lib/intl.lib" "C:/src/vcpkg/installed/x64-windows/debug/lib/exiv2.lib" "C:/src/vcpkg/installed/x64-windows/debug/lib/gio-2.0.lib" "kernel32.lib" "user32.lib" "gdi32.lib" "winspool.lib" "shell32.lib" "ole32.lib" "oleaut32.lib" "uuid.lib" "comdlg32.lib" "advapi32.lib"
LINK : warning LNK4075: ignoring '/INCREMENTAL' due to '/RELEASE' specification
gexiv2.def : error LNK2001: unresolved external symbol gexiv2_metadata_as_bytes

gexiv2\gexiv2.lib : fatal error LNK1120: 1 unresolved externals

ninja: build stopped: subcommand failed.

@talregev
Copy link
Contributor Author

I am testing version 0.14.3.
Is this symbol come after that? gexiv2_metadata_as_bytes?

@talregev
Copy link
Contributor Author

I remove this symbol from def: gexiv2_metadata_as_bytes and now it compile.
I am adding here as a patch. Thank you.

@talregev
Copy link
Contributor Author

@MonicaLiu0311 I added upstream PR as a patch. Please review it again. Don't wait few days.

@phako
Copy link

phako commented Feb 12, 2025

Ah sorry, that was against master and that symbol is indeed newer than 0.14.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phako phako phako left review comments

@Wormnest Wormnest Wormnest left review comments

@dg0yt dg0yt dg0yt left review comments

@MonicaLiu0311 MonicaLiu0311 Awaiting requested review from MonicaLiu0311

Assignees

@MonicaLiu0311 MonicaLiu0311

Labels
category:new-port The issue is requesting a new library to be added; consider making a PR!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@talregev @phako @MonicaLiu0311 @Wormnest @dg0yt