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

[libcdio]Add new port #35803

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

[libcdio]Add new port #35803

wants to merge 7 commits into from

Conversation

bwrsandman
Copy link
Contributor

@bwrsandman bwrsandman commented Dec 20, 2023

Implements #2347

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

@bwrsandman
Copy link
Contributor Author

bwrsandman commented Dec 20, 2023

Note to maintainers: This library has libtools dependencies like alsa does. I'm not sure how to manage these dependencies. Locally, errors were gone by installing texinfo.

Error due to missing dependency

Makefile:847: warning: ignoring prerequisites on suffix rule definition
Makefile:844: warning: ignoring prerequisites on suffix rule definition
/mnt/vcpkg-ci/b/libcdio/src/-release-2-eb6d86cdd9.clean/missing: line 81: makeinfo: command not found
WARNING: 'makeinfo' is missing on your system.
         You should only need it if you modified a '.texi' file, or
         any other file indirectly affecting the aspect of the manual.
         You might want to install the Texinfo package:
         <https://www.gnu.org/software/texinfo/>
         The spurious makeinfo call might also be the consequence of
         using a buggy 'make' (AIX, DU, IRIX), in which case you might
         want to install GNU make:
         <https://www.gnu.org/software/make/>
make[2]: *** [Makefile:430: .././../src/-release-2-eb6d86cdd9.clean/doc/libcdio.info] Error 127
make[1]: *** [Makefile:579: all-recursive] Error 1
make: *** [Makefile:476: all] Error 2

Should I be patching out the need for makeinfo or insert a dependency?

@JonLiu1993 JonLiu1993 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Dec 21, 2023
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.

Some early comments.

ports/libcdio/0001-Remove-doc-from-config-and-make.patch Outdated Show resolved Hide resolved
ports/libcdio/portfile.cmake Outdated Show resolved Hide resolved
ports/libcdio/portfile.cmake Outdated Show resolved Hide resolved
ports/libcdio/portfile.cmake Outdated Show resolved Hide resolved
@bwrsandman
Copy link
Contributor Author

bwrsandman commented Jan 6, 2024

I've got this somewhat compiling on windows using the same autoconf path as mac and linux... It just isn't linking properly.

However, I think I have run into a libtool bug...

libtool: link:  compile cl.exe -o .libs\\iso9660-11.dll  .libs/iso9660.obj .libs/iso9660_fs.obj .libs/rock.obj .libs/xa.obj   -nologo -W3 -utf-8 -MP -MDd -Z7 -Ob0 -Od -RTC1 -Xlinker -LC:/path/to/vcpkg/installed/x64-windows/debug/lib -Xlinker -machine:x64 -Xlinker -nologo -Xlinker -debug -Xlinker -INCREMENTAL    ../../lib/driver/.libs/cdio.lib -lkernel32 -luser32 -lgdi32 -lwinspool -lshell32 -lole32 -loleaut32 -lcomdlg32 -ladvapi32 -lwinmm "@.libs\\iso9660-11.dll.exp" -Wl,-DLL,-IMPLIB:".libs\\iso9660.lib"
LINK : warning LNK4044: unrecognized option '/LC:/path/to/vcpkg/installed/x64-windows/debug/lib'; ignored
   Creating library .libs\iso9660.lib and object .libs\iso9660.exp
iso9660_fs.obj : error LNK2001: unresolved external symbol CDIO_SECTOR_SYNC_HEADER

What's happening here is that -L is not being substituted for /LIBPATH:.

Debugging the compile command (added echo as a cl.exe command)

$ ./compile echo -LC:/path/to/vcpkg/installed/x64-windows/debug/lib
-link -LIBPATH:C:/path/to/vcpkg/installed/x64-windows/debug/lib
$ ./compile echo -L C:/path/to/vcpkg/installed/x64-windows/debug/lib
-link -LIBPATH:C:/path/to/vcpkg/installed/x64-windows/debug/lib
$ ./compile echo -Xlinker -LC:/path/to/vcpkg/installed/x64-windows/debug/lib
-link -LC:/path/to/vcpkg/installed/x64-windows/debug/lib
$ ./compile echo -Xlinker -L C:/path/to/vcpkg/installed/x64-windows/debug/lib
C:/path/to/vcpkg/installed/x64-windows/debug/lib -link -L
$ ./compile echo -I. -LC:/path/to/vcpkg/installed/x64-windows/debug/lib
-I. -link -LIBPATH:C:/path/to/vcpkg/installed/x64-windows/debug/lib
$ ./compile echo -I. -L C:/path/to/vcpkg/installed/x64-windows/debug/lib
-I. -link -LIBPATH:C:/path/to/vcpkg/installed/x64-windows/debug/lib

Clearly this shows that -Xlinker prevents the proper substitution of -L to /LIBPATH:.

I looked pretty hard in the libcdio code for where -Xlinker gets added or where compile comes from but have come up short so far.

If anyone has pointers on what to do, it would help me a great deal.

@dg0yt
Copy link
Contributor

dg0yt commented Jan 6, 2024

The -Xlinker may come from vcpkg_configure_make which is tuned for standard autotools projects with libtool and for using the compile wrapper which comes with vcpkg. And which has a quirk concerning this flag (#31228 C4).
I'm surprised to see a gnulib unistd header in the port. Isn't it included in upstream's distribution?

@bwrsandman
Copy link
Contributor Author

bwrsandman commented Jan 6, 2024

The -Xlinker may come from vcpkg_configure_make which is tuned for standard autotools projects with libtool and for using the compile wrapper which comes with vcpkg. And which has a quirk concerning this flag (#31228 C4).

Ok so that would be where it's added...

The issue about -Xlinker -L also exists outside of vcpkg_configure_make.

The compile script that consumes -Xlinker skips treatment of flags that follow it:

	-L*)
	  func_cl_dashL "${1#-L}"
	  ;;
	-Xlinker)
	  eat=1
	  linker_opts="$linker_opts $2" # skips func_cl_dashL of the next argument if it starts with -L
	  ;;

I'm surprised to see a gnulib unistd header in the port. Isn't it included in upstream's distribution?

I was surprised not to find it in upstream, because they mention a missing/ directory with stdint.h, inttypes.h and
unistd.h in MSVC/README. However, this same readme mentions msvc 2010, so it's quite old. I also looked throughout the releases for a history of these files and could not find one.

I was able to get everything compiling with a few changes which is why I haven't added a unistd.h. I may go that route later but I had a few questions about adding it to the toolchain in general. msys2 has its own unistd.h and it should be provided by gnulibs. I wonder if the header should be part of another port. I see an unofficial gnutls seems to fetch gnulibs already.

Right now I'm just trying to get things linking and packaged, then I will see about refining the patches.

@dg0yt
Copy link
Contributor

dg0yt commented Jan 6, 2024

gnulib is not meant to be used as a separate lib, but provides scripts for upstream maintainers to copy the relevant modules into upstream packages.

@JonLiu1993 JonLiu1993 changed the title Add libcdio [libcdio]Add new port Jan 8, 2024
@JonLiu1993
Copy link
Member

@bwrsandman, Is work still being done for this PR?

@bwrsandman
Copy link
Contributor Author

I would still like to get this merged but I haven't had time to work on it at the moment.

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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants