Adopt simpler strategy for big libs on Windows

[RandomShaper]

Also fixes #6844.

As explained in methods.py (also explains why it fixes #6844 ):

On Windows, due to the limited command line length, when creating a static library
from a very high number of objects SCons will invoke "ar" once per object file;
that makes object files with same names to be overwritten so the last wins and
the library looses symbols defined by overwritten objects.
By enabling quick append instead of the default mode (replacing), libraries will
got built correctly regardless the invokation strategy.
Furthermore, since SCons will rebuild the library from scratch when an object file
changes, no multiple versions of the same object file will be present.

[punto-]

I wouldn't just get rid of the code that splits the libraries, some obscure
platforms might need it, that was added for one of the consoles originally.

On 28 October 2016 at 22:37, Pedro J. Estébanez [email protected]
wrote:

Also fixes #6844 #6844.

As explained in methods.py (also explains why it fixes #6844
#6844 ):
On Windows, due to the limited command line length, when creating a static
library from a very high number of objects SCons will invoke "ar" once
per object file; that makes object files with same names to be
overwritten so the last wins and the library looses symbols defined by
overwritten objects. By enabling quick append instead of the default mode
(replacing), libraries will got built correctly regardless the invokation
strategy. Furthermore, since SCons will rebuild the library from scratch
when an object file changes, no multiple versions of the same object file

will be present.

You can view, comment on, or merge this pull request online at:

#6959
Commit Summary

• Adopt simpler strategy for big libs on Windows

File Changes

• M drivers/SCsub
  https://github.com/godotengine/godot/pull/6959/files#diff-0 (52)
• M methods.py
  https://github.com/godotengine/godot/pull/6959/files#diff-1 (10)

Patch Links:

• https://github.com/godotengine/godot/pull/6959.patch
• https://github.com/godotengine/godot/pull/6959.diff

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#6959, or mute the thread
https://github.com/notifications/unsubscribe-auth/AGVmPddgE0v6XSjDPvnm7XKT3EE_G7RTks5q4qNTgaJpZM4Kj_gE
.

[RandomShaper]

I decided to do it because of the comment:

Split drivers, this used to be needed for windows until separate builders for windows were created
FIXME: Check if still needed now that the drivers were made more lightweight

Anyway, if it's not currently used, it can be removed and it's not lost forever since you can always get it back from the repo history.

What do you think?

[punto-]

Can you leave the bool flag (false by default), and leave the code
somewhere in methods.py, but still usable when you turn the bool on? I'll
definitely be in use on some platforms.

On 28 October 2016 at 23:19, Pedro J. Estébanez [email protected]
wrote:

I decided to do it because of the comment:

Split drivers, this used to be needed for windows until separate builders
for windows were created
FIXME: Check if still needed now that the drivers were made more
lightweight

Anyway, if it's not currently used, it can be removed and it's not lost
forever since you can always get it back from the repo history.

What do you think?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#6959 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGVmPWUjOwNfrYCtkdkEm9-DHbdalA9xks5q4q0zgaJpZM4Kj_gE
.

[akien-mga]

I'd prefer we don't keep around dead code, especially as since my refactoring, the drivers lib is very lightweight, so this hack is likely not needed at all.
It might now be needed for the modules library, but if it is, it should be reintroduced when there is a proper use case IMO.

[punto-]

That's why I'm saying, we can move it to method if we want it out of the
way, and turn off by default, but it's definitely in use on some platforms.
I haven't compiled those with the new refactoring of the drivers tho..

On 29 October 2016 at 06:10, Rémi Verschelde [email protected]
wrote:

I'd prefer we don't keep around dead code, especially as since my
refactoring, the drivers lib is very lightweight, so this hack is likely
not needed at all.
It might now be needed for the modules library, but if it is, it should
be reintroduced when there is a proper use case IMO.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#6959 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGVmPXtWJk9UZv_rqHh58TtVFynHYf5-ks5q4w19gaJpZM4Kj_gE
.

[akien-mga]

Ok, sounds good. @RandomShaper Can you move the splitting function to methods.py then?

[akien-mga]

I guess that you'd have to make a methods.py function that takes as argument the env and the name of the lib to split ("drivers" or "modules" or anything else that might come in the future, maybe "thirdparty" - so I'd just make it a string). E.g.:

def split_lib(env, libname):
        import string

        num = 0
        cur_base = ""
        max_src = 64
        list = []
        lib_list = []

        for f in getattr(env, libname + "_sources"):
                fname = ""
                if type(f) == type(""):
                        fname = env.File(f).path
                else:
                        fname = env.File(f)[0].path
                fname = fname.replace("\\", "/")
                base = string.join(fname.split("/")[:2], "/")
                if base != cur_base and len(list) > max_src:
                        if num > 0:
                                lib = env.Library(libname + str(num), list)
                                lib_list.append(lib)
                                list = []
                        num = num+1
                cur_base = base
                list.append(f)

        lib = env.Library(libname + str(num), list)
        lib_list.append(lib)

        if len(lib_list) > 0:
                import os, sys
                if os.name=='posix' and sys.platform=='msys':
                        env.Replace(ARFLAGS=['rcsT'])
                        lib = env.Library(libname + "_collated", lib_list)
                        lib_list = [lib]

        lib_base=[]
        env.add_source_files(lib_base,"*.cpp")
        lib_list.insert(0, env.Library(libname, lib_base))

        env.Prepend(LIBS=lib_list)

(untested)

[punto-]

It's ok if it doesn't go in completely tested, I'll debug it eventually
when I get to that branch, just leave in place the mechanism to turn on the
functionality with that bool..

On 30 October 2016 at 12:46, Rémi Verschelde [email protected]
wrote:

I guess that you'd have to make a methods.py function that takes as
argument the env and the name of the lib to split ("drivers" or "modules"
or anything else that might come in the future, maybe "thirdparty" - so I'd
just make it a string). E.g.:

def split_lib(env, libname):
import string

    num = 0
    cur_base = ""
    max_src = 64
    list = []
    lib_list = []

    for f in getattr(env, libname + "_sources"):
            fname = ""
            if type(f) == type(""):
                    fname = env.File(f).path
            else:
                    fname = env.File(f)[0].path
            fname = fname.replace("\\", "/")
            base = string.join(fname.split("/")[:2], "/")
            if base != cur_base and len(list) > max_src:
                    if num > 0:
                            lib = env.Library(libname + str(num), list)
                            lib_list.append(lib)
                            list = []
                    num = num+1
            cur_base = base
            list.append(f)

    lib = env.Library(libname + str(num), list)
    lib_list.append(lib)

    if len(lib_list) > 0:
            import os, sys
            if os.name=='posix' and sys.platform=='msys':
                    env.Replace(ARFLAGS=['rcsT'])
                    lib = env.Library(libname + "_collated", lib_list)
                    lib_list = [lib]

    lib_base=[]
    env.add_source_files(lib_base,"*.cpp")
    lib_list.insert(0, env.Library(libname, lib_base))

    env.Prepend(LIBS=lib_list)

(untested)

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#6959 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGVmPd66gwGDJJUSfQ7kqdGDiS_tCW0Eks5q5Lu7gaJpZM4Kj_gE
.

[akien-mga]

@RandomShaper No need to rebase, I used GitHub's new security vulnerability feature that allows maintainers to push commits to the branches used in PRs of their repos :)

[akien-mga]

I've tested on Linux btw, it works. It's a bit pointless as the drivers lib is small now so it generates only libdrivers0*.a, but the logic will likely be necessary for libmodules too on those problematic platforms.

[akien-mga]

@punto- When you get to it, it might make sense to rename the bool to split_big_libs or something like that, since it will no longer be drivers-related.