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

Use ${CC} instead of "cc" for linking on mac #611

Merged
merged 1 commit into from
May 12, 2020

Conversation

nilason
Copy link
Contributor

@nilason nilason commented May 11, 2020

Fixes: #609.

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

cc -> ${CC} makes sense and it is indeed consistent with other code, but the configure file is generated from configure.in (one of the few or perhaps the only file generated which is in the repo). It is generated using autoconf-2.13. I don't see this piece in configure.in. We could merge this change, no harm in that, but the next update of configure from configure.in will remove this again.

@wenzeslaus
Copy link
Member

wenzeslaus commented May 11, 2020

I get the SHLIB_LD="cc... even with 2.69 (which is the latest version from 2012). So, patching this afterwards seems to be like the only option for fixing this here, unless somebody has a better idea. Please look at #348 for a replacement of this by CMake.

	*-apple-darwin*)
	    SHLIB_CFLAGS="-fno-common"
	    SHLIB_SUFFIX=".dylib"
	    SHLIB_LD="cc -dynamiclib -compatibility_version \${GRASS_VERSION_MAJOR}.\${GRASS_VERSION_MINOR} -current_version \${GRASS_VERSION_MAJOR}.\${GRASS_VERSION_MINOR} -install_name \${INST_DIR}/lib/lib\${LIB_NAME}\${SHLIB_SUFFIX}"
	    LD_LIBRARY_PATH_VAR="DYLD_LIBRARY_PATH"
	    ;;

@ryandesign
Copy link

Since configure is a generated file, the problem needs to be fixed in the file from which it is generated, which is aclocal.m4.

@neteler
Copy link
Member

neteler commented May 12, 2020

Since configure is a generated file, the problem needs to be fixed in the file from which it is generated, which is aclocal.m4.

Yes, here:

SHLIB_LD="cc -dynamiclib -compatibility_version \${GRASS_VERSION_MAJOR}.\${GRASS_VERSION_MINOR} -current_version \${GRASS_VERSION_MAJOR}.\${GRASS_VERSION_MINOR} -install_name \${INST_DIR}/lib/lib\${LIB_NAME}\${SHLIB_SUFFIX}"

@nilason
Copy link
Contributor Author

nilason commented May 12, 2020

Thanks a lot to all of you, really appreciate it. Pushed changes to aclocal.m4 too.

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

OK, now it makes sense to me. I don't see any further occurrences of this with grep -IrnE "SHLIB_LD[^a-z]*cc". Unless somebody adds another review or comment raising some new issues, I'm going to merge this soon. Thanks @nilason, @neteler and @ryandesign.

@wenzeslaus wenzeslaus merged commit ac07d4d into OSGeo:master May 12, 2020
@wenzeslaus
Copy link
Member

The failed Python code check is unrelated to this PR and already fixed on master (new version of Flake8 was released catching more errors). The fails on master after merging this are unrelated too (Failed to fetch https://packages.microsoft.com/ubuntu...).

@nilason nilason deleted the fix-cc-mac-609 branch May 12, 2020 20:23
@nilason
Copy link
Contributor Author

nilason commented Aug 19, 2020

@neteler Please backport this before 7.8.4.

neteler pushed a commit that referenced this pull request Aug 20, 2020
Replaces a hardcoded cc by a configured ${CC} in SHLIB_LD. Fixes #609.
@neteler
Copy link
Member

neteler commented Aug 20, 2020

@neteler Please backport this before 7.8.4.

Sure: done.

@neteler neteler added this to the 8.0.0 milestone Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] "cc" is used instead of ${CC} on Apple platforms for linking
4 participants