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

fluidsynth: use configure.cc for all components #3583

Merged
merged 1 commit into from Feb 23, 2019

Conversation

5 participants
@MarcusCalhoun-Lopez
Copy link
Contributor

MarcusCalhoun-Lopez commented Feb 5, 2019

See https://trac.macports.org/wiki/UsingTheRightCompiler

Use configure.cc to build make_tables.exe.
Since make_tables.exe is used but not installed, no revbump.

Description

Type(s)
  • bugfix
  • enhancement
  • security fix
Tested on

macOS 10.14.2
Xcode 10.1

Verification

Have you

  • checked your Portfile with port lint?
  • tried existing tests with sudo port test?
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?
fluidsynth: use configure.cc for all components
See https://trac.macports.org/wiki/UsingTheRightCompiler

Use configure.cc to build make_tables.exe.
Since make_tables.exe is used but not installed, no revbump.
@macportsbot

This comment has been minimized.

Copy link

macportsbot commented Feb 5, 2019

Notifying maintainers:
@mojca for port fluidsynth.
@RJVB for port fluidsynth.

@macportsbot

This comment has been minimized.

Copy link

macportsbot commented Feb 5, 2019

Travis Build #5157 Passed.

Lint results
--->  Verifying Portfile for fluidsynth
--->  0 errors and 0 warnings found.

Port fluidsynth success on xcode9.4. Log
Port fluidsynth success on xcode8.3. Log
Port fluidsynth success on xcode7.3. Log
Port fluidsynth success on xcode10.1. Log

ExternalProject_Add(gentables
DOWNLOAD_COMMAND ""
SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/gentables
+ CMAKE_ARGS "-DCMAKE_C_COMPILER=${CMAKE_C_COMPILER}"

This comment has been minimized.

@mojca

mojca Feb 5, 2019

Member

I would like to push this upstream.
But what about other variables / CMake arguments?

This comment has been minimized.

@MarcusCalhoun-Lopez

MarcusCalhoun-Lopez Feb 5, 2019

Author Contributor

Getting all of the CMake arguments to be passed to ExternalProject_Add seems to be nontrivial, at least according to this Stack Overflow answer.
Since the resulting program (make_tables.exe) is only used as part of the build process, perhaps the other MacPorts variable are less important?
We could also create a separate subport for make_tables.exe, but I do not know if it has any use outside of building fluidsynth.

This comment has been minimized.

@mojca

mojca Feb 6, 2019

Member

OK, after taking a closer look at https://github.com/FluidSynth/fluidsynth/blob/master/src/gentables/CMakeLists.txt

# remove $CC from the current environment and by that force cmake to look for a (working) C compiler,
# which hopefully will be the host compiler
unset(ENV{CC})

this looks like a deliberate decision. I thought it might be sufficient to comment out the line which unsets CC, but that didn't work for me and in any case I'm not sure what the correct upstream solution could be then, maybe we simply cannot solve that so easy with CMAKE, short of extending CMAKE's functionality itself.

@RJVB: using the correct compiler is part of MacPorts guidelines that allows us to run MP on legacy systems. It might not be important in this case, but what if sources start requiring C18? How would you build them on Tiger then?

I believe it makes sense to merge this. I'm not particularly proud of the way this is worked around either, but I'm short of better ideas.

@RJVB
Copy link
Contributor

RJVB left a comment

If make_tables is only used during the build process, how important is it that it is built using the compiler selected for the components that will ultimately be installed? This is C code so there is no potential libstdc++-vs-libc++ aspect and indeed I don't see a reference to a bug report.

If this is indeed the case, make_tables is a bit like GNU perf, gettext or even cmake or the compiler itself; which compiler is used to build these tools is (a priori) of no consequence for the stuff built using them.

I haven't looked at the code, but if you really think this should be built using the exact same settings it will probably be much easier to sell upstream if you get make_tables to build as a subproject instead of as an external project. IDEs that understand CMake files will then include the make_tables source code as a full part of the project, for instance.
I think this should be possible as long as the utility doesn't have to be run by cmake.

ExternalProject_Add(gentables
DOWNLOAD_COMMAND ""
SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/gentables
+ CMAKE_ARGS "-DCMAKE_C_COMPILER=${CMAKE_C_COMPILER}"

This comment has been minimized.

@RJVB

RJVB Feb 5, 2019

Contributor

Setting CMAKE_BUILD_TYPE here is at least as important as setting the compiler. We use a custom build type to make CMake use the CFLAGS & family env. variables which I can more easily imagine as more important than the actual compiler. The generated tables could be different for 32bit vs. 64bit builds for instance.

@RJVB

This comment has been minimized.

Copy link
Contributor

RJVB commented Feb 6, 2019

@mojca

This comment has been minimized.

Copy link
Member

mojca commented Feb 8, 2019

It's praiseworthy to strive for this kind of thing but by the time the problem presents itself (for this port or any other port) there might be no more users running 10.4 or interested in running the port in question

We are not yet at the point where the build fails, but some are still maintaining their own repositories just because they want to run fluidsynth on Tiger:
https://trac.macports.org/ticket/36962#comment:5

@MarcusCalhoun-Lopez

This comment has been minimized.

Copy link
Contributor Author

MarcusCalhoun-Lopez commented Feb 11, 2019

As has been noted in the upstream report, there does not seem to be a lot of enthusiasm for this patch.
@kencu is advocating for the "keep it simple" approach.

However, just as a side note, the current situation is slightly inconvenient for those of us who have set up our machines in "[h]ow to test" mode.
This is how I came across this issue in the first place.
Yes, we can revert this setup, install/upgrade fluidsynth, and then reapply.
It is just a tiny inconvenience.

@kencu

This comment has been minimized.

Copy link
Contributor

kencu commented Feb 11, 2019

Agreed -- at the moment, there is no upstream enthusiasm for changing this, so no point banging our heads against the wall.

@kencu

This comment has been minimized.

Copy link
Contributor

kencu commented Feb 11, 2019

But we can certainly fix it in MacPorts if we choose to do so.

@RJVB

This comment has been minimized.

Copy link
Contributor

RJVB commented Feb 12, 2019

@kencu

This comment has been minimized.

Copy link
Contributor

kencu commented Feb 12, 2019

Winning a point lasts a minute, but losing a friend is forever. The internet is so impersonal.

@RJVB

This comment has been minimized.

Copy link
Contributor

RJVB commented Feb 14, 2019

@mojca

This comment has been minimized.

Copy link
Member

mojca commented Feb 23, 2019

Lacking a better idea I'll merge this. I would prefer to find a better way: the sooner we find it the better, but it's now already two people who had issues updating this port and had to do workarounds, and it makes no sense to accumulate open PRs forever.

@mojca mojca merged commit f519c17 into macports:master Feb 23, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MarcusCalhoun-Lopez MarcusCalhoun-Lopez deleted the MarcusCalhoun-Lopez:fluidsynth_UsingTheRightCompiler branch Feb 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.