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

Jump to bottom

[scintilla] Add static linking to Scintilla #13529

Merged
merged 5 commits into from
Sep 22, 2020
Merged

[scintilla] Add static linking to Scintilla #13529

merged 5 commits into from
Sep 22, 2020

Conversation

alek-d
Copy link
Contributor

@alek-d alek-d commented Sep 14, 2020 •
edited
Loading

Describe the pull request

  • What does your PR fix? Fixes #

Static linking is officially supported by Scintilla. See https://www.scintilla.org/ScintillaDoc.html, at the bottom of the document, section "Building Scintilla". This PR adds static linking on Windows.

  • Which triplets are supported/not supported? Have you updated the CI baseline?

Tested successfully:
x64-windows
x64-windows-static
x64-windows-static-md
x86-windows
x86-windows-static
x86-windows-static-md

Yes, it does.

@ghost
Copy link

ghost commented Sep 14, 2020 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

@cenit
Copy link
Contributor

cenit commented Sep 14, 2020

you should not call vcpkg_apply_patches in a portfile.
Please define a symbol referring to the patch file for static or empty for dynamic and then add the PATCHES section to vcpkg_extract_source_archive_ex

Hoikas
Hoikas suggested changes Sep 14, 2020
View reviewed changes
ports/scintilla/0001-static-build.patch Outdated
<IntrinsicFunctions>true</IntrinsicFunctions>
<PreprocessorDefinitions>NDEBUG;%(PreprocessorDefinitions)</PreprocessorDefinitions>
<LanguageStandard>stdcpp17</LanguageStandard>
+ <RuntimeLibrary>MultiThreaded</RuntimeLibrary>
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes to the RuntimeLibrary force usage of the static CRT. They should be pulled into a separate patch, conditioned on VCPKG_CRT_LINKAGE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I assumed that static triplets always links CRT statically.
I've separated these two options, and now CRT can be linked dynamically.
Tested with two community triplets x64/x86-windows-static-md – works fine.
Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! 😄

ports/scintilla/portfile.cmake Outdated
vcpkg_install_msbuild(
SOURCE_PATH ${SOURCE_PATH}
PROJECT_SUBPATH Win32/SciLexer.vcxproj
INCLUDES_SUBPATH include
LICENSE_SUBPATH License.txt
ALLOW_ROOT_INCLUDES
)

vcpkg_copy_pdbs()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please include a trailing newline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Included.

ras0219-msft
ras0219-msft requested changes Sep 14, 2020
View reviewed changes
ports/scintilla/portfile.cmake
@@ -13,10 +11,20 @@ vcpkg_extract_source_archive_ex(
REF 4.2.3
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
)
PATCHES ${PATCHES}
)

You can then conditionally set PATCHES based on VCPKG_CRT_LINKAGE:

if(VCPKG_CRT_LINKAGE STREQUAL "static")
  set(PATCHES 0001-static-build.patch)
else()
  set(PATCHES)
endif()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the tip.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ras0219-msft could you please approve the change. I would like to update Scintilla to the newest version in another PR, but this one is hanging for so long.

@PhoebeHui PhoebeHui changed the title Add static linking to Scintilla [scintilla] Add static linking to Scintilla Sep 15, 2020
@PhoebeHui PhoebeHui added category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist requires:author-response labels Sep 15, 2020
@alek-d alek-d requested a review from ras0219-msft September 15, 2020 22:26
@PhoebeHui PhoebeHui added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Sep 17, 2020
@cenit
Copy link
Contributor

cenit commented Sep 17, 2020

scintilla:x64-windows-static=fail in ci.baseline.txt, but in reality is also described as skip

            scintilla:x64-windows-static:       skip: 3e9793440929e680a883b0c2d392ba56798acdbb

this PR should fix this problem and let CI test the port maybe?

@alek-d
Copy link
Contributor Author

alek-d commented Sep 17, 2020 •
edited
Loading

@cenit Thanks. ci.baseline.txt updated.

I have a question about other triplets. Maybe all other currently marked as fail should be marked as skip.
Current Scintilla portfile.cmake starts with vcpkg_fail_port_install(ON_TARGET "Linux" "OSX" "UWP").

So, maybe it would be better to just skip those build rather than expecting them to fail?
Should I change to this?

# Currently Scintilla builds only on Windows
scintilla:arm-uwp=skip
scintilla:x64-linux=skip
scintilla:x64-osx=skip
scintilla:x64-uwp=skip

@cenit
Copy link
Contributor

cenit commented Sep 17, 2020

No leave them as fail.
Skip is only for ports conflicting with each other (name conflicts in header or lib filenames)

@alek-d
Copy link
Contributor Author

alek-d commented Sep 22, 2020

Removed vcpkg_copy_pdbs() from portfile.cmake because it is called in vcpkg_install_msbuild().

@alek-d alek-d mentioned this pull request Sep 22, 2020
Merged
@ras0219-msft ras0219-msft merged commit 9a7b70b into microsoft:master Sep 22, 2020
@ras0219-msft
Copy link
Contributor

LGTM, sorry for the long delay on this PR and thanks for addressing my comment!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hoikas Hoikas Hoikas requested changes

@PhoebeHui PhoebeHui PhoebeHui approved these changes

@ras0219-msft ras0219-msft Awaiting requested review from ras0219-msft

Assignees

@PhoebeHui PhoebeHui

Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@alek-d @cenit @ras0219-msft @Hoikas @PhoebeHui