search

PCRE2Project / pcre2

PCRE2 development is now based here.
Other
919 stars 191 forks source link

fix MSVC PDB installation #370

Open cosine0 opened 10 months ago

cosine0 commented 10 months ago

fix #369

PhilipHazel commented 10 months ago

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

carenas commented 10 months ago

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 commented 10 months ago

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 (https://github.com/PCRE2Project/pcre2/commit/f7b89f94a2f9e8c6ad84a12e60390137f1135232), 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 commented 10 months ago

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.

zherczeg commented 1 week ago

Is this patch still valid/needed?