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

[mpg123] Allow cross-compiling, fix previous builds tainting rebuilds #11535

Merged
merged 9 commits into from
Aug 1, 2020

Conversation

kevinlul
Copy link
Contributor

Closes #11164

This would support cross-compiling to triplets like arm64-linux from x64 if an appropriate cross-compiling configuration or toolchain file is specified in the triplet.

I fixed a bug where previous builds were not cleaned up when building again. This came up when testing and I commented it; this could be separated into a different pull request.

I refactored the port a little bit to have common configure flags shared between debug and release builds; again, this could be in a different pull request.

Now for the meat of the problem I'm trying to solve: in the linked issue, JackBoosY suggests using the new vcpkg_configure_make. However, I found that this actually sets a bad combination of default flags for mpg123, resulting in attempting to macOS-specific CoreAudio code on Linux. I also noticed that it only attempts to detect autoconf --host if on Windows/MSYS, so I went with my own approach here of retrieving the prefix with -dumpmachine and passing it to configure and make.

@JackBoosY
Copy link
Contributor

@Neumann-A Could you review this PR?

Thanks.

@Neumann-A
Copy link
Contributor

The port needs a complete rewrite to use vcpkg_configure_make() and vcpkg_install_make()
a) use SET(ENV{CC} <somecompiler>) in the triplet or have it already be set in the environment which vcpkg uses. vcpkg_configure_make should automatically pick it up. (or more precisely configure should pick it up under linux)
b) (unreleated to the pr) the prefix passed to configure is completely wrong. Using vcpkg_configure_make() would solve part of that.
c) about the --host option VCPKG is currently missing a proper way to pass --build=something --target=something --host=something to configure. Even though it is currently not handled for linux in vcpkg_configure_make you can always use OPTIONS to pass these. I am thinking of adding something like VCPKG_(AUTOTOOLS_)CONFIGURE_(MAKE_)TRIPLET which is intended to be set be the users to specify --build=something --target=something --host=something in a custom triplet. I am still missing a proper name for that variable.

Conclusion:
a) Rewrite port to use vcpkg_configure_make() and vcpkg_install_make()
b) add a global variable to vcpkg_configure_make to specify --build=something --target=something --host=something which can be used for cross-compiling cases in custom triplets. (Because it should affect all autotools based ports. )

@kevinlul
Copy link
Contributor Author

The default set of flags used by vcpkg_configure_make cause the build to fail. I tried to use it but as I noted, I found that this actually sets a bad combination of default flags for mpg123, resulting in attempting to macOS-specific CoreAudio code on Linux.

@Neumann-A
Copy link
Contributor

I found that this actually sets a bad combination of default flags for mpg123, resulting in attempting to macOS-specific CoreAudio code on Linux.

Be more specific. If this is only due to --host this can be changed in vcpkg_configure_make as mentioned in my comment before

@kevinlul
Copy link
Contributor Author

When I had tested, it seemed to be the result of the combination of --disable-silent-rules with --disable-shared added here

@Neumann-A
Copy link
Contributor

When I had tested, it seemed to be the result of the combination of --disable-silent-rules with --disable-shared added here

Test again. The configure script has both rules baked into it so it should work.... (Otherwise check your autoconf version 2.69 is required)

@kevinlul
Copy link
Contributor Author

By removing a bunch of flags, I got it to compile properly. Tomorrow I will verify that this is still working on macOS. This has a behaviour change: libmpg123.la and libout123.la are now installed to lib directories. Should these be removed?

Some thoughts:

  • the make distclean could be moved to vcpkg_configure_make because it seems like it could affect other ports that have in-source builds too
  • setting the --host triplet via -dumpmachine could also be moved to vcpkg_configure_make so more autoconf ports can be easily cross-compiled with vcpkg

@kevinlul
Copy link
Contributor Author

I've verified that it compiles successfully on macOS, including the CoreAudio module.

ports/mpg123/portfile.cmake Outdated Show resolved Hide resolved
ports/mpg123/portfile.cmake Outdated Show resolved Hide resolved
@JackBoosY JackBoosY added category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist requires:author-response labels Jun 15, 2020
kevinlul and others added 2 commits June 15, 2020 11:06
Co-authored-by: Jack·Boos·Yu <47264268+JackBoosY@users.noreply.github.com>
@kevinlul kevinlul requested a review from JackBoosY June 17, 2020 02:03
Copy link
Contributor

@JackBoosY JackBoosY left a comment

Choose a reason for hiding this comment

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

Please update the version info. See documentation.

@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Jun 17, 2020
Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

LGTM if the merge from master doesn't break anything.

ports/mpg123/portfile.cmake Outdated Show resolved Hide resolved
@JackBoosY JackBoosY removed the info:reviewed Pull Request changes follow basic guidelines label Jul 14, 2020
@kevinlul
Copy link
Contributor Author

Turns out all those lines were unnecessary holdovers from the old version of this port and it's all taken care of by vcpkg_configure_make and vcpkg_install_make. Nifty!

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Jul 22, 2020
@strega-nil
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@strega-nil
Copy link
Contributor

I like this PR quite a lot! let's just run the PR tests and see if we can merge :)

@JackBoosY
Copy link
Contributor

CMake Error at scripts/cmake/vcpkg_find_acquire_program.cmake:377 (message):
  Could not find yasm.  Please install it via your package manager:

      brew install yasm
Call Stack (most recent call first):
  ports/mpg123/portfile.cmake:39 (vcpkg_find_acquire_program)
  scripts/ports.cmake:79 (include)


Error: Building package mpg123:x64-osx failed with: BUILD_FAILED
Elapsed time for package mpg123:x64-osx: 1.314 s

@JackBoosY JackBoosY added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Jul 30, 2020
@strega-nil
Copy link
Contributor

Looks like this needs to update the scripts.

@JackBoosY
Copy link
Contributor

I don't think mpg123 should use yasm in *nix, please change

vcpkg_find_acquire_program(YASM)
get_filename_component(YASM_EXE_PATH ${YASM} DIRECTORY)
set(ENV{PATH} "$ENV{PATH};${YASM_EXE_PATH}")

to

if (VCPKG_TARGET_IS_WINDOWS)
    vcpkg_find_acquire_program(YASM)
    get_filename_component(YASM_EXE_PATH ${YASM} DIRECTORY)
    set(ENV{PATH} "$ENV{PATH};${YASM_EXE_PATH}")
endif()

@kevinlul
Copy link
Contributor Author

The assembler is used on all platforms if it is available. On my macOS CI I have to brew install yasm before installing mpg123. Turning that off would mean that we are not using the hand-optimized assembly on those platforms.

@JackBoosY JackBoosY added category:infrastructure Pertaining to the CI/Testing infrastrucutre and removed requires:author-response labels Jul 31, 2020
@strega-nil strega-nil added the info:reviewed Pull Request changes follow basic guidelines label Jul 31, 2020
@strega-nil strega-nil merged commit 5512eaf into microsoft:master Aug 1, 2020
@kevinlul kevinlul deleted the mpg123-arm branch August 1, 2020 22:41
hellozee pushed a commit to hellozee/vcpkg that referenced this pull request Sep 11, 2020
…microsoft#11535)

* [mpg123] Allow cross-compiling, fix previous builds tainting rebuilds

* [mpg123] Rewrite to use vcpkg_configure_make

* Update ports/mpg123/portfile.cmake

Co-authored-by: Jack·Boos·Yu <47264268+JackBoosY@users.noreply.github.com>

* [mpg123] Use vcpkg_execute_required_process

* [mpg123] Bump CONTROL version

* [mpg123] Remove unneeded legacy lines and strip out trailing newline from dumpmachine

* pre-install yasm on OSX.

Co-authored-by: Jack·Boos·Yu <47264268+JackBoosY@users.noreply.github.com>
Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
Co-authored-by: JackBoosY <yuzaiyang@beyondsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:infrastructure Pertaining to the CI/Testing infrastrucutre category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[mpg123] Building on ARM
5 participants