fix MSVC PDB installation

[cosine0]

fix #369

[PhilipHazel]

As I know nothing about Windows, and very little about CMake, would anybody else like to comment on this patch? @carenas ?

[carenas]

comment on this patch?

sorry, had been busy and far from Windows to verify that oue setup is indeed broken there but it might had been a result of the latest revert so a historical look at the code is definitely needed.

the fact that CI was also patched and was working before mght seem to indicate this is more a "preference".

definitely got confused by the description in the linked ticket, which might be worth expanding on, like is this using the cmake from VS, or one that was installed on top?, which version?, were older versions tested to behave the same?, what version of PCRE is affected and is this a regession?

[cosine0]

I believe this is not a matter of a regression or preference. This behavior occurs both in the default GitHub CI environment and my local setup, which is VS 2022 with the separately installed CMake.
CMake, since version 3.1, have had the toolkit-independent "generator expressions" $<TARGET_PDB_FILE:tgt>, $<TARGET_PDB_FILE_NAME:tgt>, and $<TARGET_PDB_FILE_DIR:tgt> for getting the path where the PDB files are generated.
But, ${PROJECT_BINARY_DIR} used instead in the exsiting CMakeLists.txt in this repo, which has been there since the introduction of CMake (f7b89f9), is not specifically supposed to be used for finding the PDB files, because sometimes PDB files are not present there.
My patch replaces ${PROJECT_BINARY_DIR} into $<TARGET_PDB_FILE_DIR:tgt> to ensure PDB files are definitely found. This works for both the VS 2022 generator and Ninja generator, and maybe for other generators too.

[carenas]

I see now, so cmake --install is broken and this seems to address only one part of it, FWIW the pdb files get generated in the RelWithDebInfo directory in my setup (using the embedded cmake that comes with VS 2022 Preview)

Ideally the changes to CI could be avoided and an additional "dev" job that builds a DLL might be added to try to excercise this code path, but the logic seem to be broken as it assumes that "prefix/bin" will be the place where the binaries should be, which looks suspiciously like a GNU standard and not a Windows one.

[carenas]

CI uses build.yml for "production" like settings, so it is better to create a new job that would use VS in windows as part of "dev" builds.

[carenas]

Instead of changing this CI job, add a new one in dev.yml that addresses this specific dev configuration

[carenas]

is this really needed?

[carenas]

"bin" might change based on other cmake variables, so it might be better to make that setting explicit at configure time to avoid fragility

[zherczeg]

Is this patch still valid/needed?

[NWilson]

Yes, this is still needed. The patch is good - I'll open a new PR with a rebased version of it (to save @cosine0 from having to come back to this).

Many thanks @cosine0 - your fix does work, I apologise it wasn't merged sooner.

Stale review, I have rebased

[NWilson]

Many thanks @cosine0 for reporting and fixing this! I apologise for the long time it took us to respond and accept the contribution.

[cosine0]

Sorry for responding late. And thank you for reviewing and accepting this!