Manually linking static libraries with MSVC appends Godot's LIBSUFFIX

[rxlecky]

Added functions append_libs() and prepend_libs() that add libraries to the $LIBS environment variable. It deduces and adds full path of each library to the $LIBS.

This solves issue #23687.

Since I'm not very experienced with Python, the code will probably need some clean up. All comments are welcome.

[akien-mga]

Since those are only used in get_libs_full_path, you can add them at the start of the function directly.

[rxlecky]

Good point; will fix that.

[rxlecky]

Fixed

[akien-mga]

Thanks! I think it would be good to have input from upstream SCons contributors on this, as it's starting to look like we need a lot of own code for something that should be provided by SCons IMO.

[rxlecky]

Yeah, it would be definitely nice if this was supported directly by SCons. I meant to post an issue at their repo and this would be a temporary solution till it gets implemented on their side.

[rxlecky]

Alright, finally got it to build on all platforms.

Now, this is quite a hacky implementation, for example it manipulates the link command:

godot/SConstruct

Lines 442 to 444 in d59767e

	# Disabling prefix and suffix concatenation to library names.
	# Libraries are added as full paths using append/prepend_libs function.
	env["_LIBFLAGS"] = '${_concat("", LIBS, "", __env__)}'

but this is what I was able to come up with. Any suggestions are welcome.

Also, the three added functions treat the libs parameter slightly differently on Windows and on the other (linux) platforms

godot/methods.py

Lines 686 to 720 in 3f6602e

	def append_libs_windows(self, libs, libpaths = []):
	libs = get_libs_full_path(self, libs, libpaths)
	self.Append(LIBS=libs)
	def prepend_libs_windows(self, libs, libpaths = []):
	libs = get_libs_full_path(self, libs, libpaths)
	self.Prepend(LIBS=libs)
	def insert_libs_windows(self, index, libs, libpaths = []):
	libs = get_libs_full_path(self, libs, libpaths)
	try:
	self["LIBS"][index:index] = libs
	except KeyError:
	if index == 0:
	self.Append(LIBS=libs)
	else:
	raise IndexError
	def append_libs_linux(self, libs, libpaths = []):
	self.Append(LIBPATH=libpaths)
	self.Append(LIBS=libs)
	def prepend_libs_linux(self, libs, libpaths = []):
	self.Prepend(LIBPATH=libpaths)
	self.Prepend(LIBS=libs)
	def insert_libs_linux(self, index, libs, libpaths = []):
	self.Prepend(LIBPATH=libpaths)
	try:
	self["LIBS"][index:index] = libs
	except KeyError:
	if index == 0:
	self.Append(LIBS=libs)
	else:
	raise IndexError

On Windows, it is only used for lookup of that specific libraries, whereas on other platforms it is added to the $LIBPATH. It is not a big difference, but it can cause issues with builds - Linux user can rely on the paths being added to the $LIBPATH, but on Windows it would break. So, would you recommend removing the parameter completely?

One last thing, I'd also rename the functions append_libs_linux to just append_libs since that's the implementation for all platforms other than Windows.

[akien-mga]

I'll move this to 3.2 milestone, 3.1-beta is around the corner and I want to take the time to review this in-depth and discuss with upstream SCons devs to see if there could be a cleaner way to achieve this.

[rxlecky]

Agreed, this is quite a big change so we better be careful with it.

[akien-mga]

I still have the feeling that we're working around our own bug here, while it should be possible to get things working properly with SCons. If not, we should possibly look into debugging SCons itself.

I haven't had/taken time to investigate further sadly... so moving to the next milestone, to investigate further after the 3.2 release. Now I have a box with Windows 10 and MSVC at least, so I can try to debug myself :)

[rxlecky]

Yeah, this sure feels like we are hacking our way around. I think it would be useful to get input on this from SCons devs so that we know whether we are missing something or whether this is not yet implemented in SCons. If some work still needs to be done on our end, I'm happy to help out with it. I kinda forgot about this issue so I didn't really do much work on it after the first couple of commits.

[aaronfranke]

@rxlecky Is this still desired? If so, it needs to be rebased on the latest master branch.

If not, abandoned pull requests will be closed in the future as announced here.

[rxlecky]

Not entirely sure. Did we have a chance/are we still planning to discuss this with the SCons team @akien-mga?

[aaronfranke]

This PR has not received any new commits for over a year and is abandoned, closing.

If this is still desired, it can be re-opened or re-created after it is rebased. I would suggest discussing with @akien-mga first, but I don't want to leave an inactive un-mergeable PR open for so long.