Windows Builders: Cmake build improvements

Having trouble installing or compiling FreeCAD? Get help here.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
wmayer
Founder
Posts: 20243
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Windows Builders: Cmake build improvements

Post by wmayer »

Some questions and remarks:

At the moment there is an automatic check to enable BULD_QT5 if the environment variable FREECAD_LIBPACK_DIR is set pointing to a libpack directory. I think this can be extended to the case if someone (like I usually do) set FREECAD_LIBPACK_DIR in the CMake GUI before doing the initial Configure step.

Now in your PR you split the IF/ELSE block into two sections

Code: Select all

      IF(EXISTS ${FREECAD_LIBPACK_DIR}/lib/Qt5Core.lib)
        OPTION(BUILD_QT5 "Build with Qt5." ON)
      ENDIF()
and

Code: Select all

        # Default Qt5 ON if present in libpack
        IF(EXISTS ${CMAKE_BINARY_DIR}/bin/Qt5Core.dll)
            OPTION(BUILD_QT5 "Build with Qt5." OFF)
        ENDIF()
Especially I don't understand the logic of the latter block. Why does it default the value to OFF when it knows that it found Qt5Core.dll? And why does it search inside build directory and not in the libpack?

Then the general problem with the PR is that BUILD_QT5 is only set in case CMake found some Qt5 specific files. However, I still use some older libpacks with a different directory structure where the above automatism doesn't work and thus won't offer the BUILD_QT5 option. So, it effectively becomes (almost) impossible that the user can override the default because it's not even offered.

So, in this case it's better to keep and improve the current logic.

Not sure why a user wants to have the shiboken2.exe in the build directory.

Code: Select all

        # Copy libpack 'bin' to build 'bin' per user request - if non-existant at destination
        IF(NOT EXISTS ${CMAKE_BINARY_DIR}/bin/Scripts/shiboken2.exe)
            SET(COPY_LIBPACK_BIN_TO_BUILD ON)
            OPTION(FREECAD_COPY_LIBPACK_BIN_TO_BUILD "Copy larger libpack dependency 'bin' folder to the build direcctory." OFF)
        ENDIF()
IMO the message about copying time and disk space is irrelevant and it mainly depends on the HW. So, a hint that the copy operation can take a couple of minutes should be sufficient.

Code: Select all

                MESSAGE("=======================================\n"
                    "Copying libpack bin directory to build directory for Windows MSVC build.\n"
                    "This will take a while... (5-10 minutes or longer)\n"
                    "Time depends upon whether libpack is Release(1 GB), Debug(1 GB+), or BOTH(2 GB+).\n")

Code: Select all

                MESSAGE("To auto-copy the 'bin' directory from the libpack to the build directory,\n"
                    "toggle the FREECAD_COPY_LIBPACK_BIN_TO_BUILD parameter to ON and configure again.\n"
                    "NOTE: The copy process can take from 5-15 minutes depending on your libpack installed.\n")
Too much irrelevant information, IMO.

Code: Select all

    ELSE()
        MESSAGE("If you intend to use the Windows libpack, set the FREECAD_LIBPACK_DIR to the libpack directory.")
        MESSAGE(STATUS "As of 23 March 2019, APeltauer released three configurations of libpack for Windows: 
            Debug, Release, and Combined (Debug and Release).\n"
            "Visit: https://github.com/apeltauer/FreeCAD/releases/ for downloads.")
Actually the CMake logic should not be too tight to a certain libpack version because this makes things much harder for people who use their own libpacks or that from 3rd parties.
User avatar
sgrogan
Veteran
Posts: 6499
Joined: Wed Oct 22, 2014 5:02 pm

Re: Windows Builders: Cmake build improvements

Post by sgrogan »

Thanks for reviewing wmayer.
wmayer wrote: Fri May 24, 2019 2:27 pm Not sure why a user wants to have the shiboken2.exe in the build directory.
I think this is meant as a check if the libs have already been copied, i.e, an incremental build. In the powershell script that I use, from peterl94, he looks for $buildDir\bin\QtCore4.dll which is also problematic. What's the best guess that the .dll's have been copied previously?
wmayer wrote: Fri May 24, 2019 2:27 pm At the moment there is an automatic check to enable BULD_QT5 if the environment variable FREECAD_LIBPACK_DIR is set pointing to a libpack directory. I think this can be extended to the case if someone (like I usually do) set FREECAD_LIBPACK_DIR in the CMake GUI before doing the initial Configure step.
+1, this would be very good for me as well.
"fight the good fight"
wmayer
Founder
Posts: 20243
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Windows Builders: Cmake build improvements

Post by wmayer »

I think this is meant as a check if the libs have already been copied, i.e, an incremental build. In the powershell script that I use, from peterl94, he looks for $buildDir\bin\QtCore4.dll which is also problematic. What's the best guess that the .dll's have been copied previously?
For a normal user it's probably easier to create a FreeCAD.bat file that extends the PATH with the LibPack directory. This doesn't require to copy over any files.

And for the package maintainers it might be easier if there was a .cmake file offered by the LibPack directly that holds a list of the needed files to create the installer.
Russ4262
Posts: 941
Joined: Sat Jun 30, 2018 3:22 pm
Location: Oklahoma
Contact:

Re: Windows Builders: Cmake build improvements

Post by Russ4262 »

Afternoon, Gentlemen.
Mr. Mayer,
Thanks for the remarks and valuable time to review the purposed changes in the PR. I have addressed the items you mentioned in your comments.
wmayer wrote: Fri May 24, 2019 2:27 pm Now in your PR you split the IF/ELSE block into two sections

Code: Select all

      IF(EXISTS ${FREECAD_LIBPACK_DIR}/lib/Qt5Core.lib)
        OPTION(BUILD_QT5 "Build with Qt5." ON)
      ENDIF()
and

Code: Select all

        # Default Qt5 ON if present in libpack
        IF(EXISTS ${CMAKE_BINARY_DIR}/bin/Qt5Core.dll)
            OPTION(BUILD_QT5 "Build with Qt5." OFF)
        ENDIF()
Especially I don't understand the logic of the latter block. Why does it default the value to OFF when it knows that it found Qt5Core.dll? And why does it search inside build directory and not in the libpack?
The referenced 'OFF' setting was an error. Nonetheless, I have removed this block entirely in the now updated version.

wmayer wrote: Fri May 24, 2019 2:27 pm Then the general problem with the PR is that BUILD_QT5 is only set in case CMake found some Qt5 specific files. However, I still use some older libpacks with a different directory structure where the above automatism doesn't work and thus won't offer the BUILD_QT5 option. So, it effectively becomes (almost) impossible that the user can override the default because it's not even offered.
The BUILD_QT5 option is available to all users, as I understand the code. It is presented just a few lines above the Windows-specific code blocks we are are discussing.

Code: Select all

# Switch to build FreeCAD with Qt5
OPTION(BUILD_QT5 "Build with Qt5." OFF)
wmayer wrote: Fri May 24, 2019 2:27 pm So, in this case it's better to keep and improve the current logic.
I have worked to do just that, Sir. Or, at least make fewer errors than in the original version.



wmayer wrote: Fri May 24, 2019 2:27 pm Not sure why a user wants to have the shiboken2.exe in the build directory.

Code: Select all

        # Copy libpack 'bin' to build 'bin' per user request - if non-existant at destination
        IF(NOT EXISTS ${CMAKE_BINARY_DIR}/bin/Scripts/shiboken2.exe)
            SET(COPY_LIBPACK_BIN_TO_BUILD ON)
            OPTION(FREECAD_COPY_LIBPACK_BIN_TO_BUILD "Copy larger libpack dependency 'bin' folder to the build direcctory." OFF)
        ENDIF()

@SGrogan stated it as well as I could:
sgrogan wrote: Fri May 24, 2019 7:17 pm Thanks for reviewing wmayer...
I think this is meant as a check if the libs have already been copied, i.e, an incremental build...
What's the best guess that the .dll's have been copied previously?

wmayer wrote: Fri May 24, 2019 2:27 pm IMO the message about copying time and disk space is irrelevant and it mainly depends on the HW. So, a hint that the copy operation can take a couple of minutes should be sufficient...
Too much irrelevant information, IMO.
Okay, Sir. I trimmed down the message content to users...

But, perhaps you would like to know that as a teacher, a part of my profession is to assist my students in becoming independent learners. A part of that teaching process requires inserting strategic, useful information at the appropriate time and location in a lesson. So, I tend to want to include extra comments and messages for users so that they can proceed with built-in support mechanisms(messages and comments). In my line of work, the more I invest in providing built-in, appropriate and necessary feedback before and during a designated independent learning task, the better the experience will be for the learner/user.

I really do appreciate the fine work you do and wonderful software you hold together. I am enjoying both working and developing with FreeCAD. Thanks for your decade(??) of investment in FreeCAD and the FC community.

Russell
wmayer
Founder
Posts: 20243
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Windows Builders: Cmake build improvements

Post by wmayer »

I had to do the merging all manually since too much has changed in the meantime. A few things I reworked a little bit and hope it didn't damage too much :)
git commit d887735820
wmayer
Founder
Posts: 20243
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Windows Builders: Cmake build improvements

Post by wmayer »

The BUILD_QT5 option is available to all users, as I understand the code. It is presented just a few lines above the Windows-specific code blocks we are are discussing.
Yes, but the problem is that the option is used inside an IF-check and if this is not true the option won't be added.
But, perhaps you would like to know that as a teacher, a part of my profession is to assist my students in becoming independent learners. A part of that teaching process requires inserting strategic, useful information at the appropriate time and location in a lesson.
Well, it might be OK for newbies but for the others who know better how things work it can be annoying to have a lot of irrelevant information as it makes it harder to find the relevant information.
wmayer
Founder
Posts: 20243
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Windows Builders: Cmake build improvements

Post by wmayer »

And here is the proposed idea to improve handling of the BUILD_QT5 option: git commit e2d273406

This also fixes a minor regression of the last commit which defined BUILD_QT5 before the further checks.
Russ4262
Posts: 941
Joined: Sat Jun 30, 2018 3:22 pm
Location: Oklahoma
Contact:

Re: Windows Builders: Cmake build improvements

Post by Russ4262 »

wmayer wrote: Sat May 25, 2019 1:18 pm I had to do the merging all manually since too much has changed in the meantime. A few things I reworked a little bit and hope it didn't damage too much :)
git commit d887735820
Thank you, Sir, for your time, assistance, and support.

I appreciate it.

Russell
Post Reply