Page MenuHome

fix T53951: copy runtime dlls using build in cmake functionality.
ClosedPublic

Authored by LazyDodo (LazyDodo) on Jan 30 2018, 9:15 PM.

Details

Summary

not sure why we were doing it by hand, cmake seems to have a handle on things.

That being said, do we want to keep doing this? @Ton Roosendaal (ton) seems to be a fan of not having to download anything, however people have taken offense with it in the past ( T48109 )

I can probably come up with a simple detection and make blender gracefully exit when the required run-time is not found on the system (perhaps with a nice popup on where to obtain the runtime)

the BF will have to decide what they want.

Diff Detail

Repository
rB Blender

Event Timeline

This patch description is quite vague,

not sure why we were doing it by hand, cmake seems to have a handle on things.

The other comments imply users might have to download runtime DLL's.

In the past - reports from users having to download anything additional has been a continual pain point (Python specifically, but MSVC runtime was an issue too IIRC). Not sure why we would go back to this unless they are for sure available on modern systems.

This patch description is quite vague,

not sure why we were doing it by hand, cmake seems to have a handle on things.

InstallRequiredSystemLibraries is a module that ships with cmake, it has one function. "copy the runtime dlls to the bin folder" our buildscripts imported the module, and used some of it's internal variables to do the copying our selves.

Given the internal vars of InstallRequiredSystemLibraries are not standardized official cmake variables, cmake was free to change them, eventually did breaking our buildscripts.

this patch removes the copying we were doing, and relies on the functionality InstallRequiredSystemLibraries offers.

end result is the same, all the dll's required for running blender will end up next to the blenders .exe, and no additional files need to be downloaded.

@LazyDodo (LazyDodo) from your last comment I don't see why we wouldn't apply this patch (if it really does work as intended).

But am left wondering how your comments about users downloading runtime's or having improved error messages when the run-time isn't found.

Is there some chance cmake will fail to copy runtime libs in the future, so there is some use in having improved detection for this case?


If this never impacts users (only CMake maintenance), then I don't see anything against applying the patch.

LazyDodo (LazyDodo) added a comment.EditedFeb 1 2018, 12:54 AM

I brought that up earlier today on irc, the BF stance was blender should work on a clean install, no extra downloads needed, so we're gonna keep shipping the runtime dll's (even though they ballooned from 3-4 to about 45 files)

If this never impacts users (only CMake maintenance), then I don't see anything against applying the patch.

actually it does, i accidentally shipped the 2.79a-rc without the runtime dll's due to this issue, just nobody has noticed it yet...

If this never impacts users (only CMake maintenance), then I don't see anything against applying the patch.

actually it does, i accidentally shipped the 2.79a-rc without the runtime dll's due to this issue, just nobody has noticed it yet...

Probably this is not a good test case, people testing the RC's will most likely be technical users with multiple builds already running - not be new users with a clean install.

AFAICS existing functionality should be kept then.

Just as a clarification, cmake silently removed the runtime dlls for 2.79a, i didn't do this deliberately, This patch retains the old functionality (include all needed dll's), and no longer silently fails when build with newer cmake.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 1 2018, 4:26 PM
This revision was automatically updated to reflect the committed changes.