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

[libvhdi] Initial Port #36063

Merged
merged 32 commits into from
Feb 10, 2024
Merged

[libvhdi] Initial Port #36063

merged 32 commits into from
Feb 10, 2024

Conversation

TomKatom
Copy link
Contributor

@TomKatom TomKatom commented Jan 6, 2024

  • 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.

@TomKatom
Copy link
Contributor Author

TomKatom commented Jan 6, 2024

@microsoft-github-policy-service agree

@microsoft-github-policy-service agree

@dg0yt
Copy link
Contributor

dg0yt commented Jan 7, 2024

Why not just build with vcpkg_configure_make?

@TomKatom
Copy link
Contributor Author

TomKatom commented Jan 7, 2024

vcpkg_configure_make

It's preferred to use CMake when possible, this library is quite small so it is manageable to do so.
Also, it would make life easier when compiling for Windows and etc...

I took inspiration from the libvmdk port, its by the same library author so it's pretty much the same.
Although if you've got a better idea on how do this better I'm open to ideas.

Thanks,
Tom

@dg0yt
Copy link
Contributor

dg0yt commented Jan 7, 2024

It is preferred to use the build system maintained upstream.

@TomKatom
Copy link
Contributor Author

TomKatom commented Jan 7, 2024

It is preferred to use the build system maintained upstream.

Got it, now using autoconf and msbuild.

@FrankXie05 FrankXie05 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Jan 8, 2024
This reverts commit 94e0c97.
@TomKatom TomKatom marked this pull request as ready for review February 4, 2024 22:16
ports/libvhdi/Config.cmake.in Outdated Show resolved Hide resolved
@FrankXie05 FrankXie05 added the info:reviewed Pull Request changes follow basic guidelines label Feb 7, 2024
Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

I still think it should use its native build system. Maybe even autotools for windows.

"license": "LGPL-3.0-or-later",
"supports": "!uwp",
"dependencies": [
"gettext",
Copy link
Contributor

Choose a reason for hiding this comment

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

No, this isn't what you need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do you think that? According to the libvhdi build requirements gettext is needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

What is needed? libintl? gettext tools?

Copy link
Contributor

Choose a reason for hiding this comment

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

And often it can be disabled with autotools.

Copy link
Contributor

Choose a reason for hiding this comment

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

What is needed? libintl? gettext tools?

Still not answered.

@TomKatom
Copy link
Contributor Author

TomKatom commented Feb 7, 2024

I still think it should use its native build system. Maybe even autotools for windows.

The thing is, autotools is not the "native build system" for this library.
This library along with all the other libyal libraries (3 of them are ported in this exact same way already mind you) has a very odd windows compilation instructions which require running a custom python script to convert the vcproj files into vcxproj files for MSBuild compilation.

I'm open to suggestions on how to overcome that barrier and compile using MSBuild for windows and autotools for Unix, but as far as I am aware there's no simple way to build vcproj using vcpkg.
But the status quo for the libyal library suite ports is this solution so I think that just for consistency alone this cmake port is the way to go.

@dg0yt
Copy link
Contributor

dg0yt commented Feb 8, 2024

So for all platforms but MSVC, autotools is known to work. And for MSVC, we didn't try to use it.

@data-queue data-queue merged commit 2cb7bfd into microsoft:master Feb 10, 2024
16 checks passed
@dg0yt
Copy link
Contributor

dg0yt commented Feb 10, 2024

Who cares about guidelines?
This port violates the guidelines also with regard to the exported CMake config. It must be moved to the unofficial namespace.
(I didn't want to review the CMake code before it is necessary.)

@dg0yt
Copy link
Contributor

dg0yt commented Feb 17, 2024

There are four ports now from https://github.com/libyal/: libpff, libqcow, libvmdk, libvhdi.
The individual git repos contain only specific code, but the tarball assets share extra subdirs which are actually other repos / tiny libs there.
Overview: https://github.com/libyal/libyal/wiki/Overview

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants