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.

Sign up for GitHub

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
Closed

Update build docs for windows #528

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,27 @@ install

Please refer to the CMake Manual for more details.

**Windows**:
Different configurations have different run-time requirements. The `Debug`
configuration uses the windows debug C runtime, whereas the `RelWithDebInfo`
does not. This may determine which version you can use with your executable.

Some special notes regarding `cmake`:
- cmake doesn't need the configuration int the `-B` step since it creates
all the `*.vcxproj` files at once.
Comment on lines +121 to +122
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 •
edited
Loading

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 •
edited
Loading

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 :)

- for `cmake --build`, **do no** specify `--parallel`, but **do** specify your
`--config RelWithDebInfo` or whichever configuration is appropriate.
Comment on lines +123 to +124
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:

sentry-native/tests/cmake.py

Lines 138 to 141 in 06d4360

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 •
edited
Loading

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 short, do something like
```
> cmake -B build
> cmake --build build --config RelWithDebInfo
> cmake --install build --prefix install --config RelWithDebInfo
```

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.

Already handled in https://github.com/getsentry/sentry-native/blob/master/CMakeLists.txt#L343-L366

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.


**Android**:

The CMake project can also be configured to correctly work with the Android NDK,
Expand Down