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

Update build docs for windows #528

Closed
wants to merge 1 commit into from

Conversation

kristjanvalur
Copy link

Some windows build tips gathered by trial and error.
Probably needs formatting and entering to whatever source editor was used to create the Readme :)

Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

left some comments, looks good otherwise.

Comment on lines +121 to +122
- cmake doesn't need the configuration int the `-B` step since it creates
all the `*.vcxproj` files at once.
Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by that? The -B flag specifies the output directory of the VC project, otherwise it would just clutter the working directory.

Copy link
Author

@kristjanvalur kristjanvalur Apr 23, 2021

Choose a reason for hiding this comment

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

Sorry, that was unclear. You don't need the -DCMAKE_BUILD_TYPE=RelWithDebInfo setting there, although it is harmless. All the configurations are embedded in the resulting .vcxproj files.

Copy link
Contributor

@Mixaill Mixaill Apr 23, 2021

Choose a reason for hiding this comment

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

You do not need it in case of MSVS generator or other multi-configuration projects, but you can use -G Ninja for example. In case of Ninja or Make you should specify build type.

Copy link
Author

Choose a reason for hiding this comment

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

Right you are. So it may well be left in place though. It is harmless enough :)

Comment on lines +123 to +124
- for `cmake --build`, **do no** specify `--parallel`, but **do** specify your
`--config RelWithDebInfo` or whichever configuration is appropriate.
Copy link
Member

Choose a reason for hiding this comment

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

Its unfortunate that --parallel is just ignored on Windows. I found that you actually need to set a cflag to have parallel compilation like so:

if sys.platform == "win32":
# MP = object level parallelism, WX = warnings as errors
cpus = os.cpu_count()
cflags.append("/WX /MP{}".format(cpus))

Though I’m not really sure if there might be a better way here. Documentation is quite bad :-(

Copy link
Author

@kristjanvalur kristjanvalur Apr 23, 2021

Choose a reason for hiding this comment

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

Right but the point I was trying to make was that if you do add --parallel, then the build step will only create the Debug configuration and ignores --config. Took me half an hour of experimentation last night to figure out what was going on. If you leave out the --parallel flag, then it will build whatever configuration you specify with --config

Copy link
Member

Choose a reason for hiding this comment

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

ohhh, I think in that case it might be a different manifestation of #512, which means that cmakes option parsing is broken

Copy link
Author

Choose a reason for hiding this comment

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

could be. Don't know what the best solution is. Only that I am not an avid cmake user myself and always struggle a bit when using it. Wanted to help save people from the problems I had :)

```

In addition to the optional `winhttp.lib`, windows builds require linking
with `version.lib` and `dbghelp.lib`
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Right, but it probably ought to be pointed out to users of the sentry.lib library, who otherwise need to hunt through their linker errors to figure out which windows libraries to include. It is already mentioned that winhttp.lib is a dependency but the others aren't mentioned.

@kristjanvalur
Copy link
Author

So, what do we do with this? Looking at Readme.md, it looks to me as it comes from some external editor. This PR was not really meant for direct pulling, rather as a suggestion for updates.

@Swatinem
Copy link
Member

Its just how my vscode (or actually prettier) autoformatted that, so no problem. I can also just copy-paste this in.

@kristjanvalur
Copy link
Author

kristjanvalur commented Apr 27, 2021 via email

@supervacuus
Copy link
Collaborator

The raised topics in this PR concerning the CMake config/build parameters when using the MSBuild generator are all valid points. But I think we covered them in the docs a while back by providing more contrasting build examples: https://docs.sentry.io/platforms/native/#install

@supervacuus supervacuus closed this Feb 8, 2023
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.

4 participants