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

[libxmp] add new port #30799

Merged
merged 1 commit into from
Apr 15, 2023
Merged

[libxmp] add new port #30799

merged 1 commit into from
Apr 15, 2023

Conversation

dbarkar
Copy link
Contributor

@dbarkar dbarkar commented Apr 12, 2023

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

@FrankXie05 FrankXie05 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Apr 13, 2023
ports/libxmp/vcpkg.json Show resolved Hide resolved
ports/libxmp/portfile.cmake Show resolved Hide resolved
@FrankXie05
Copy link
Contributor

I need to test these features and wait for the CI check to finish.

@FrankXie05
Copy link
Contributor

The features and usage have been tested successfully locally.

@FrankXie05 FrankXie05 added the info:reviewed Pull Request changes follow basic guidelines label Apr 14, 2023
@BillyONeal BillyONeal merged commit d4acc3c into microsoft:master Apr 15, 2023
@BillyONeal
Copy link
Member

Thanks for the new port!

@BillyONeal
Copy link
Member

This broke in CI because it conflicts with the existing port, libxmp-lite. It looks like libxmp[core] is equivalent to libxmp-lite; should we just delete the old port and point customers at this one?

I note that sdl2-mixer has a find_dependency(libxmp-lite) which suggests cmake configs might be different?

@BillyONeal
Copy link
Member

/cc @Ghabry You added the original libxmp-lite port back in 2017, does changing to use this one make sense?

@JackBoosY
Copy link
Contributor

Also, the source downloaded from sourceforge is far outdated than the one on github:
image

@HunterZ
Copy link

HunterZ commented Apr 18, 2023

Also, the source downloaded from sourceforge is far outdated than the one on github

I was worried about the same thing, but then I noticed that @dbarkar (who opened this issue) has also recently contributed to the libxmp github repo. I'm therefore assuming that vcpkg is building the github source and not older, apparently abandoned sourceforge source.

@dbarkar
Copy link
Contributor Author

dbarkar commented Apr 18, 2023

I just tried to build latest libxmp-lite from github and it is still in conflict with libxmp - they both install xmp.h.

Also, libxmp-lite lite will be deprecated in future - libxmp/libxmp#387.

should we just delete the old port and point customers at this one?

I suggest to do so :)

@JackBoosY
Copy link
Contributor

I just tried to build latest libxmp-lite from github and it is still in conflict with libxmp - they both install xmp.h.

Yeah it should always have conflicts.
Can we build libxmp-lite from libxmp source? If it's yes, which cmake options should we pass into vcpkg_cmake_configure?

Also, libxmp-lite lite will be deprecated in future - libxmp/libxmp#387.

should we just delete the old port and point customers at this one?

I suggest to do so :)

Yeah so we prefer to remove this port.
But instead of this, we can also simply add feature lite to libxmp and point libxmp-lite to libxmp[lite] feature.

@dbarkar
Copy link
Contributor Author

dbarkar commented Apr 18, 2023

Can we build libxmp-lite from libxmp source? If it's yes, which cmake options should we pass into vcpkg_cmake_configure?

It would require to use different CMakeLists.txt, from lite directory. Will it work for vcpkg?

@JackBoosY
Copy link
Contributor

JackBoosY commented Apr 18, 2023

Can we build libxmp-lite from libxmp source? If it's yes, which cmake options should we pass into vcpkg_cmake_configure?

It would require to use different CMakeLists.txt, from lite directory. Will it work for vcpkg?

When I checked the CMakeLists of libxpm and libxpm-lite, I thought the solution I said earlier was unreasonable:

  • If the library generated by libxmp-lite will always conflict with the library generated by libxmp, we can't install both library files at the same time (the conflict will happen in the msbuild integration).
  • We had to make a huge patch of cmakelists to do all of this request.

So, I think we should remove this port again, do you agree this?

@dbarkar
Copy link
Contributor Author

dbarkar commented Apr 18, 2023

I agree.
Users of xmp-lite can use xmp without depackers and loaders (exposed as features). It wouldn't require changes in the code.

@Ghabry
Copy link
Contributor

Ghabry commented Apr 18, 2023

I'm fine with removing xmp-lite.

The reason why I havn't bumped xmp-lite yet is, that there is still no released version of the library with official CMake support in the upstream repo.

There is only one when you use the main/master branch directly as you did. Thanks for this.


The main reason why I use "xmp-lite" is the license. lite is MIT I think and xmp is LGPL.

Will suggest to upstream to have this as a build type option, then it can be a feature opt-in. (what @JackBoosY suggested)

@BillyONeal
Copy link
Member

Thanks folks!

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.

6 participants