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

Building OpenSCAD with Visual Studio #3957

Closed
wants to merge 70 commits into from

Conversation

dannypike
Copy link

Instructions and a few modifications to support building and debugging OpenSCAD from within Microsoft Visual Studio on a windows PC. See the file VisualStudio.md for details.

…atible types elsewhere, when it tries to copy VectorOfVector2d types around.

Perhaps this was a requirement in earlier versions of MSVC?
…id a duplicate ID error during the build. Not sure why it's trying to load the same manifest twice, or even if this workaround will cause problems elsewhere but this hack allows it to link
…d run a Debug build under Visual Studio 2019. At this stage, it will load a basic .scad project but cannot find the shaders, so there is something wrong with the PlatformUtils resource path.
…d run a Debug build under Visual Studio 2019. At this stage, it will load a basic .scad project but cannot find the shaders, so there is something wrong with the PlatformUtils resource path.
…f the CMake install() command. When 'ON', It causes CMake to copy the resources to the 'build' folder, rather than the 'install' folder. This will then allow the OpenSCAD executable to find its resources when it is running under the Visual Studio debugger.
…able; it matches the standard VCPKG name for this information
@t-paul t-paul requested a review from thehans October 28, 2021 11:05
@t-paul
Copy link
Member

t-paul commented Nov 7, 2021

Thanks for the update and sorry for the current churn on the build stuff. I just want to mention this is a very much appreciated change. It might take a bit more time for someone to review but it will be great to have.

@dannypike
Copy link
Author

You're welcome. I'll try to keep it in synch with the master branch, and vcpkg updates, as they appear.

find_package(PkgConfig)

# Note: Saving CMAKE_MODULE_PATH as CGAL will overwrite it.
# Reconsider this after CGAL 5.4: https://github.com/CGAL/cgal/pull/6029
if ("${CMAKE_MODULE_PATH}" STREQUAL "")
Copy link
Member

Choose a reason for hiding this comment

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

This whole section is doing some odd things. CMAKE_MODULE_PATH Is actually a list of paths to search over, not a single path, so I don't think there should be any check for empty string.
The previous behavior was actually appending a path to the existing value, instead of completely replacing it.

If you look at the automated builds (click the red X next to your commits), you can see that MXE builds have been failing to find libzip since your changes. This is likely because the MXE toolchain sets its own CMAKE_MODULE_PATH, and so our additional path is not appended here.

If a platform/toolchain, such as VCPKG or MXE provides Find[package].cmake files etc, its best to use those before our own. (Ideally we wouldn't need to maintain our own at all, if every platform had all the needed find modules).

set(ORIGINAL_CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/cmake/Modules")
set(CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/cmake/Modules")
else()
message(STATUS "Saving CMAKE_MODULE_PATH '${ORIGINAL_CMAKE_MODULE_PATH}'")
Copy link
Member

Choose a reason for hiding this comment

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

This message is printing the value of a variable before it is set.

find_package(unofficial-fontconfig CONFIG REQUIRED)
list(APPEND COMMON_LIBRARIES unofficial::fontconfig::fontconfig)
find_package(fontconfig REQUIRED)
list(APPEND COMMON_LIBRARIES ${VCPKG_LIBRARY_DIR}/fontconfig.lib)
Copy link
Member

Choose a reason for hiding this comment

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

The whole point of find_package is that it sets various variables eg [packageName]_LIBRARIES, so that you don't have to construct their paths yourself.

In this case vcpkg was previously providing a package with unofficial in the name, but it seems they must have dropped that recently. Still there should be either standard variable name set, or a namespaced target (with double colons :: like in the removed code)

target_include_directories(OpenSCAD SYSTEM PRIVATE ${GLIB2_INCLUDE_DIRS})
list(APPEND COMMON_LIBRARIES unofficial::glib::gio unofficial::glib::glib unofficial::glib::gmodule unofficial::glib::gobject)
list(APPEND COMMON_LIBRARIES
Copy link
Member

Choose a reason for hiding this comment

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

Use namespaced library targets

find_library(GETTEXT_LIBRARY libintl)
list(APPEND COMMON_LIBRARIES ${GETTEXT_LIBRARY})
find_library(GETTEXT_LIBRARY intl)
list(APPEND COMMON_LIBRARIES ${VCPKG_LIBRARY_DIR}/intl.lib) # In MSVC, 'libintl' is called 'intl'
Copy link
Member

Choose a reason for hiding this comment

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

find_library sets GETTEXT_LIBRARY to the path, so use it.

…, as that is now handled by having a manifest file (vcpkg.json). Added a description of the debugging process, once the build is complete.
…cpkg steps are now handled automatically by the manifest file (vcpkg.json).
…cpkg steps are now handled automatically by the manifest file (vcpkg.json).
…nto vs2019-port

# Conflicts:
#	doc/Windows-VisualStudio.md
…ifest, to the minimum required for vcpkg to optimise the build
… to start up with the x64-Release seleced by default. Otherwise it will try to build the initial configuration with x86-Debug.
@thehans
Copy link
Member

thehans commented Feb 14, 2022

@dannypike your PR is showing 487 files changed now. There was a major reformatting of the codebase recently (#4095) that touched pretty much every source file (mostly whitespace, settling on a standard indentation model), so it seems that something went wrong during one of your latest merges.

I also added some code review comments regarding your changes in CMakeLists.txt. Up to a certain point there were no changes to the various cmake/modules/Find[package].cmake files, and then suddenly you modified a number of them. I feel these are unnecessary changes (since it was working for you before) as long as you don't completely overwrite vcpkg's CMAKE_MODULE_PATH.

@t-paul
Copy link
Member

t-paul commented Feb 14, 2022

Sorry, I did not expect the code formatting to cause such issues. As this has progressed quite a bit, how is that suggestion:

I'll try to rebase this onto latest master branch and we import the changes into a dedicated branch in the openscad repo.
That way it might also be a bit easier trying to setup a github action running this (at least I believe that's possible). I would not want to run that later on each commit, but a schedule like once a week or daily if we get enough caching could be quite usable too.

@dannypike
Copy link
Author

dannypike commented Feb 15, 2022

@thehans I'm sorry about the number of commits; it's a side-effect of the way that I want to check that my changes to the build instructions match what actually happens when a user downloads and tries to build OpenSCAD on a new machine. As @t-paul says, I think that these are caused by me keeping this branch in sync with the master by back-merging them.

You can rest assured that I will never deliberately reformat any of the OpenSCAD files (I hate it when people do that, too!) and I will not change any of the logic without either: raising a separate ticket to talk about it, or by adding or removing #if _MSVC_VER gates around my modification. Every other change to this port will have come from a fetch-and-remerge of the from the master branch.

As for the current state of this vs2019-port branch:

As I have learned more about the inner workings of OpenSCAD, I have been simplifying and adjusting the build instructions as much as I can. This is partly because I don't want to scare off new users and partly because I don't want to generate work for myself later, in the form of support tickets. By automating as much of the process as I can using a manifest file, '.vs' configurations etc, I think I have got it quite close to saying please clone this repo and hit F5'. If I can get the TL;DR` section down to three or four simple steps, then I will be happy.

To avoid breaking things, as I only have a short amount of time each day, I use Git to do this in very small steps and test each one by cloning on a clean VM. That means there are lots of small commits that you can safely ignore for the moment. When I have something ready for you to check, I will write a commit that mentions you directly, and detail the things that you need to compare against the last image that you checked, so you can do a git diff between the new "published" HEAD and your last comments.

Incidentally, this is the reason why I have not yet reacted to your comments of "12 days ago". I haven't forgotten about them, I only want to keep them "safe and separate" from this storm caused by my many small adjustments to build instructions.

I hope that makes sense!

Dan

@dannypike
Copy link
Author

dannypike commented Feb 15, 2022

Sorry, I did not expect the code formatting to cause such issues. As this has progressed quite a bit, how is that suggestion:

I'll try to rebase this onto latest master branch and we import the changes into a dedicated branch in the openscad repo. That way it might also be a bit easier trying to setup a github action running this (at least I believe that's possible). I would not want to run that later on each commit, but a schedule like once a week or daily if we get enough caching could be quite usable too.

@t-paul Please give me a few days to finish what I am doing to the build instructions. Then I will work through the comments that @thehans suggested and it will be fine for you to merge the branch with master.

The Visual Studio build for OpenSCAD have been working for a quite a few days, now. Hans' comments relate more to doing things in the correct style; they are not so much about build failures with Visual Studio on his machine. When I have finished this current burst of changes to the build, it should be safe for you to merge and I can then fix any new "style" problem reports from Hans separately.

I will announce when I am happy for you to merge the pull request.

…ion in Visual Studio, rather than adding a user-configuration file to git
…cpkg steps are now handled automatically by the manifest file (vcpkg.json).
@dannypike
Copy link
Author

The git tree for this seems to have become corrupted somewhere around 67ccb8b. The graph says that should have been a merge commit, but it isn't one. so a whole bunch of files are now missing and it won't build.

So, I'm closing this and will do a fresh PR, by manually copying over my changes to a new fork of the master branch. That will have two benefits: it'll get me out of this hole that I seem to have fallen into and it will also make the PR a lot cleaner for you guys to analyse. There are very few files to copy over, despite the long history: almost all of the changes have been confined to README.md, doc/* and CMakeLists.txt, so this seems like the most sensible thing to do.

@dannypike
Copy link
Author

This PR has now been superceded by PR 4126. I shall copy-quote comments from here to that PR, as appropriate.

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.

None yet

3 participants