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

fix MSVC PDB installation #370

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cosine0
Copy link

@cosine0 cosine0 commented Jan 16, 2024

fix #369

@PhilipHazel
Copy link
Collaborator

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

@carenas
Copy link
Contributor

carenas commented Jan 17, 2024

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
Copy link
Author

cosine0 commented Jan 18, 2024

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
Copy link
Contributor

carenas commented Jan 19, 2024

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.

Copy link
Contributor

@carenas carenas left a comment

Choose a reason for hiding this comment

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

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.

@@ -104,13 +104,16 @@ jobs:
uses: actions/checkout@v3

- name: Configure
run: cmake -DPCRE2_SUPPORT_JIT=ON -DPCRE2_BUILD_PCRE2_16=ON -DPCRE2_BUILD_PCRE2_32=ON -DCMAKE_IGNORE_PREFIX_PATH=C:/Strawberry/c -B build -A Win32
run: cmake -G "Visual Studio 17 2022" -DBUILD_SHARED_LIBS=ON -DINSTALL_MSVC_PDB=ON -DPCRE2_SUPPORT_JIT=ON -DPCRE2_BUILD_PCRE2_16=ON -DPCRE2_BUILD_PCRE2_32=ON -DCMAKE_IGNORE_PREFIX_PATH=C:/Strawberry/c -B build -A Win32
Copy link
Contributor

Choose a reason for hiding this comment

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

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

..\..\RunTest.bat
./pcre2posix_test -v
..\..\build\RelWithDebInfo\pcre2posix_test -v
Copy link
Contributor

Choose a reason for hiding this comment

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

is this really needed?


- name: Test
run: |
cd build\Debug
cd install\bin
Copy link
Contributor

Choose a reason for hiding this comment

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

"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
Copy link
Collaborator

Is this patch still valid/needed?

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.

Install fails with INSTALL_MSVC_PDB=ON and -G "Visual Studio XXX"
4 participants