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

Build enhancement for scintilla static linkage #9594

Closed
wants to merge 11 commits into from

Conversation

chcg
Copy link
Contributor

@chcg chcg commented Feb 28, 2021

See #9574:

Don mentioned he would "look into static linking" of Scintilla and thus by inference Boost.

@donho Adapted VS build to link against static libscintilla.lib. Boost path in Vvcxproj is just hardcoded to the appveyor path C:\Libraries\boost_1_69_0\lib32|64-msvc-14.1\ and mingw is not adapted yet.

So just a first testversion as proof of concept to show what needs to be adapted. The build result is usable and seems to show no problem during my first tests.

@Uhf7
Copy link
Contributor

Uhf7 commented Mar 4, 2021

seems to show no problem...

For me, it seems like syntax highlighting doesn't work anymore. Only plain black texts.

@chcg
Copy link
Contributor Author

chcg commented Mar 4, 2021

@Uhf7 I must have been blind at the time I took a look at it. Indeed the highlighting is missing.
Checking the makefile libscintilla.lib is similar to Scintilla.dll, but N++ uses SciLexer.dll and therefore a static equivalent is missing.
I added one named liblexer.lib and now hightlighting/"lexing" is also working.
Thanks for spotting this.

@donho donho self-assigned this Mar 4, 2021
@Uhf7
Copy link
Contributor

Uhf7 commented Mar 4, 2021

Syntax highlighting works now, and the size of the new Notepad++.exe file is in the range expected for a unified image of the classic Notepad++.exe+SciLexer.dll.

@chcg chcg force-pushed the appveyor_static_linkage branch 2 times, most recently from 4f35526 to 82487b5 Compare March 4, 2021 22:50
Copy link
Member

@donho donho left a comment

Choose a reason for hiding this comment

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

  1. Modify Scintilla register error message.
  2. Make boost lib path more generic.

@@ -122,6 +122,7 @@
<SubSystem>Windows</SubSystem>
<TargetMachine>MachineX86</TargetMachine>
<MinimumRequiredVersion>5.01</MinimumRequiredVersion>
<AdditionalLibraryDirectories>..\..\scintilla\bin;C:\Libraries\boost_1_69_0\lib32-msvc-14.1\</AdditionalLibraryDirectories>
Copy link
Member

Choose a reason for hiding this comment

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

This path C:\Libraries\boost_1_69_0\lib32-msvc-14.1\ is for Appveyor only.
Could we use a more generic directory like ..\..\boost.pcreLib\ instead, so in both devs' env and Appveyor, we copy boost regexpr lib previously ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the preferable solution would be:
https://social.msdn.microsoft.com/Forums/vstudio/en-US/c5fa62ac-e84f-4d61-a9bf-eb8136305393/creating-a-static-libraries-that-depends-on-other-libraries?forum=vclanguage

  • modify the build of libscilexer.lib to already contain the dependent libs of boost, imm32, msimg32.lib within the static one

Also possible:
See https://social.msdn.microsoft.com/Forums/en-US/2b2e070c-2918-49ed-a64e-7ef413272e98/how-to-include-vc-directory-paths-defined-in-microsoftcppuserprop-file-in-msbuild-command-line?forum=msbuild

  • We could add a prop file and define there the boost path

  • add some default path which is working for manual usage and configure appveyor via commandline

  • use nuget packages for boost within N++

Copy link
Member

Choose a reason for hiding this comment

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

@chcg
Ideally it would be great to add Scintilla project into the solution and the compiled boost regexpr static lib into repo, so any dev can pull the repo and build Notepad++ with whole features via VS2017(VS2019 in the future)'s Build Solution command.

But so far I'm trying to find a middle term solution for both dev (like you & me) and Appveyor build system. So C:\Libraries\boost_1_69_0\lib32-msvc-14.1\ cannot be used, because it's only for build system. The middle term solution for me is this PR with a empty boost lib directory (without version number) in repository (a placeholder dummy file inside it), plus the additional build instruction to tell users to put the compiled boost pcre lib into this directory.
In this way we have the solution now for both dev & build environment, and users can tests new binary immediately.
What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@donho Did I get you right that a only VS solution (N++ with scilexer) would be the goal?
In this case we had already something like that, see below. The boost stuff in that case is coming from a nuget package (https://github.com/sergey-shandar/getboost) which contains prebuilt binaries, taken from https://dl.bintray.com/boostorg/release/1.72.0/binaries/ and is automatically downloaded by VS studio. This could be also easily adapted for the static build.

What do you think about "Unicode Release" vs. just "Release"

Revision: eba913d
Author: Christian Grasser christian.grasser@live.de
Date: 13.03.2017 21:28:02
Message:
Scintilla Namespace

  • corrected missing scintilla namespaces
  • activated usage of scintilla namespace in nmakefile and vcxproj

Closes #3033

Modified: scintilla/boostregex/AnsiDocumentIterator.h
Modified: scintilla/boostregex/UTF8DocumentIterator.cxx
Modified: scintilla/boostregex/UTF8DocumentIterator.h
Modified: scintilla/lexers/LexObjC.cxx
Modified: scintilla/lexers/LexSearchResult.cxx
Modified: scintilla/win32/SciLexer.vcxproj
Modified: scintilla/win32/scintilla.mak

Revision: fdd2dbc
Author: Christian Grasser christian.grasser@live.de
Date: 11.06.2015 17:37:54
Message:

  • add npp boostregex dir/sources and define SCI_OWNREGEX
  • add boost via nuget package

Modified: scintilla/win32/SciLexer.vcxproj
Added: scintilla/win32/packages.config

Revision: a6e0dd9
Author: Christian Grasser christian.grasser@live.de
Date: 11.06.2015 11:31:59
Message:
adapted scintilla vs project to notepad++ naming of build configurations from
Debug -> Unicode Debug and Release -> Unicode Release, to hav a consistent look in VS solution
Used same
ToolsVersion="12.0"
and
v120_xp

Modified: scintilla/win32/SciLexer.vcxproj

Copy link
Contributor

@mere-human mere-human Mar 6, 2021

Choose a reason for hiding this comment

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

Ideally it would be great to add Scintilla project into the solution and the compiled boost regexpr static lib into repo, so any dev can pull the repo and build Notepad++ with whole features via VS2017(VS2019 in the future)'s Build Solution command.

Just an idea. In theory, CMake could be used for that without the need of a project/solution file.
I think, it could allow building the entire N++ in "one click".
It should handle any subprojects whether they're using Make or anything else.
It is integrated right into Visual Studio: https://docs.microsoft.com/en-us/cpp/build/cmake-projects-in-visual-studio?view=msvc-160#ide-integration
Adding a NuGet package to the CMake setup or any other package (e.g. vcpkg) should be relatively easy.
I can research this more and provide more info with a full list of pros/cons in a separate issue, if anybody is interested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The nuget regex package contains also pre-built boost libs. And the same issue appears on using the ones from appveyor, so we remain with the problem to add an additional path to the boost libs into the N++ vcxproj file. I will try to find out why it is not sufficient to have the static lib be part of libscilexer.lib. This is working for e.g. imm32.lib.

Copy link
Member

Choose a reason for hiding this comment

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

@chcg
Sorry I wasn't clear. By "include pre-built boost lib", I meant do without nuget package since you said "This requires the nuget package also in the n++ vcxproj files.".

My vision to the future solution is:

Solution notepadPlus (notepadPlus.sln)
    Notepad++ (notepadPlus.vcxproj) - generate notepad++.exe (include libscilexer.lib)
    Scintilla (scintilla/win32/libSciLexer.vcxproj) - generate libscilexer.lib (include boostRegexpr.lib)

boostRegexpr.lib could be pre-built and included in the repository. In this way we have complete chain to build notepad++.exe, Building Notepad++ will be much easier for whomever - it needs only to fork the project locally, launch the solution then "Build the solution" or "Build Notepad++" command.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@donho There are interesting things ongoing with boost 1.76 beta1, see https://www.boost.org/users/history/version_1_76_0.html. Regex will be changed to be a header only version.

Regex:

Regex is now header only except in C++03 mode.
Support for C++03 is now deprecated.
The library can now be used "standalone" without the rest of Boost being present.

So maybe that will make it easier to use it for use. In the future.

Additionally there is already https://www.boost.org/doc/libs/1_75_0/doc/html/xpressive.html available, which is also header only and might be worth a look, if updated regex doesn't evolve in the right direction.

For now I will try to modify the PR and add the binary libs directly into git. This would be
libboost_regex-vc141-mt-s-x32-1_72.lib 12 MB
libboost_regex-vc141-mt-s-x64-1_72.lib 16 MB

I hope I got your proposal right.

Copy link
Member

@donho donho Mar 19, 2021

Choose a reason for hiding this comment

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

The library can now be used "standalone" without the rest of Boost being present.

If that does mean boost regexpr contains only it-self, why don't we include it in the repository directly?

Solution notepadPlus (notepadPlus.sln)
    Notepad++ (notepadPlus.vcxproj) - generate notepad++.exe (include libscilexer.lib)
    Scintilla (scintilla/win32/libSciLexer.vcxproj) - generate libscilexer.lib (include boostRegexpr.lib)
    BoostRegExpr (BoostRegExpr/boostRegExpr.vcxproj) generate boostRegexpr.lib

I hate git sub-Module and don't like the idea of Nuget - my goal is make build process as simple as possible so a 80 year-old grand-mom or a 8 year-old kid can build Notepad++ without RTFM. And with such simplicity, the binary generated by Appveyor will be near the binary of release - that's a great gain IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@donho Just spotted the boost beta today, so I didn't have a closer look on it yet. So it is unclear to me what "standalone" means exactly.

Maybe you like subtree more than submodule:
https://blog.developer.atlassian.com/the-power-of-git-subtree/?_ga=2-71978451-1385799339-1568044055-1068396449-1567112770

As long as you are staying within VisualStudio nuget packages are hidden behind the scenes for the "end user" and just pressing F7 is doing the build + checkin that the package exists. So not too far away from your expectation.

throw std::runtime_error("ScintillaEditView::init : SCINTILLA ERROR - Can not load the dynamic library");
if (!Scintilla_RegisterClasses(hInst))
{
throw std::runtime_error("ScintillaEditView::init : SCINTILLA ERROR - Can not load the dynamic library");
Copy link
Member

Choose a reason for hiding this comment

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

This error message should be modified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to:
ScintillaEditView::init : SCINTILLA ERROR - Scintilla_RegisterClasses failed

PowerEditor/visual.net/notepadPlus.vcxproj Outdated Show resolved Hide resolved
@sasumner
Copy link
Contributor

sasumner commented Mar 5, 2021

@chcg Very nice progress so far.

Somewhat unrelated but I'll ask because you're in there working on the build...

The .exe "artifacts" have a space in their names, e.g. Notepad++.x64.UnicodeSPACERelease.exe
Possible/reasonable to change to Notepad++.x64.Unicode.Release.exe ?
I just like the symmetry of the . replacing the space. :-)

How long do we carry along the baggage of "Unicode" in the name? (... and truly elsewhere in the project's embedded names also).
We have no "non-unicode" build anymore, so?

@chcg
Copy link
Contributor Author

chcg commented Mar 5, 2021

@sasumner The name is created in appveyor.yml by:

    $nppFileName = "Notepad++.$env:PLATFORM_INPUT.$env:CONFIGURATION.exe"

As the space is part of the configuration name this could be changed by some additional env variable or easier as you suggested to remove the unicode from the config name.

@chcg chcg force-pushed the appveyor_static_linkage branch 2 times, most recently from cb0c2ab to 2dd086c Compare March 14, 2021 09:28
@chcg
Copy link
Contributor Author

chcg commented Mar 21, 2021

@donho With the last checkin I removed the nuget again and replaced it with the new header only boost regex from 1.76 beta1. Seems to be a quite easy solution which works for nmake build and also one stop VS solution.

  • There is an older regression for the mingw build since the change of the xml config file to UTF-8.
  • mingw build should be also be possible to use boost, which is not done yet

@donho
Copy link
Member

donho commented Mar 22, 2021

@chcg

With the last checkin I removed the nuget again and replaced it with the new header only boost regex from 1.76 beta1. Seems to be a quite easy solution which works for nmake build and also one stop VS solution.

Sorry, I don't quite follow you. Did you add the project (and source code) of boost regex 1.76 beta1 into notepad_plus solution?

@chcg
Copy link
Contributor Author

chcg commented Mar 22, 2021

@donho See 6c64e55. I added the stripped regex folder from the boost distribution 1.76 beta1 under scintilla/boostregex/boost. Just the new header only V5 version as we don't need c++03 support.
58 new files of ~770 kB.

The build always is done with boost regex and has no longer the option to switch based on the existence of boost path and libs environment vars.
Mingw in the meantime also compiles with that new boost version.

If that way is ok, I think we need some testing for the regex functionality to be sure that nothing is broken with this new version or maybe we should wait at least for the boost 1.76 release and see if some bugs are still found in the regex component.
Also the build readme needs some update then.

@donho
Copy link
Member

donho commented Mar 23, 2021

@chcg
So boost regexpr is headers only?
I've compiled 32 bits and it does the job - that's really cool!
I'll do more tests on binary to make sure there's no regression and no performance issue.

I think we need some testing for the regex functionality to be sure that nothing is broken with this new version or maybe we should wait at least for the boost 1.76 release and see if some bugs are still found in the regex component.

I think we should wait for the official release to include it. In the meanwhile, we can test the binary & refine the PR.

Bravo!

edit: I see the ARM build is in the config, it's great - Notepad++ will support ARM in the future, if it's possible.

@chcg
Copy link
Contributor Author

chcg commented Mar 23, 2021

@donho
It is ARM64 which needs VS2017 15.9 (https://blogs.windows.com/windowsdeveloper/2018/11/15/official-support-for-windows-10-on-arm-development/). It was there in the vcxproj file already of scilexer dll config. It would require the N++ counterpart from PR #6196. Should I merge it into this PR and close the older one?

We might see some migration issues between arm and x86 at runtime, see https://docs.microsoft.com/en-us/cpp/build/common-visual-cpp-arm-migration-issues?view=msvc-160. So we should get some guys from #5158 who have the necessary win10 on arm to test it.

@donho
Copy link
Member

donho commented Mar 23, 2021

It would require the N++ counterpart from PR #6196. Should I merge it into this PR and close the older one?

Yes please.

We might see some migration issues between arm and x86 at runtime, see https://docs.microsoft.com/en-us/cpp/build/common-visual-cpp-arm-migration-issues?view=msvc-160. So we should get some guys from #5158 who have the necessary win10 on arm to test it.

Once PR #6196 is merged into this PR and it does be compiled, we will ask them for testing.

@chcg chcg force-pushed the appveyor_static_linkage branch 2 times, most recently from e0b02ca to e5a2bf5 Compare March 25, 2021 05:05
@chcg
Copy link
Contributor Author

chcg commented Mar 25, 2021

Fixed merge conflict with change from #9566

  • fix to have boost::wrapexcept() available -> added further files from boost for exception, assert, config
  • added missing #include <boost/throw_exception.hpp>
  • fix compiler errors/warnings from boostregexsearch files for mingw

See https://www.boost.org/doc/libs/1_75_0/libs/exception/doc/frequently_asked_questions.html maybe it makes sense to use
current_exception_diagnostic_information(); for the unknown case at
https://github.com/notepad-plus-plus/notepad-plus-plus/blob/master/scintilla/boostregex/BoostRegExSearch.cxx#L339

@chcg chcg mentioned this pull request Mar 25, 2021
@donho
Copy link
Member

donho commented Mar 25, 2021

@chcg
Just tried to compile in ARM64, but got the error message:
error MSB8006: The Platform for project 'libSciLexer.vcxproj' is invalid. Platform='ARM64'. This error may also appear if some other project is trying to follow a project-to-project reference to this project, this project has been unloaded or is not included in the solution, and the referencing project does not build using the same or an equivalent Platform. 1>Done building project "libSciLexer.vcxproj" -- FAILED.

My VS version is 15.9.15:
image

Do you have any idea for this error?

@chcg
Copy link
Contributor Author

chcg commented Mar 25, 2021

@donho Seems you have not installed the arm64 compiler for VS2017

grafik

See also
https://www.youtube.com/watch?v=OZtVBDeVqCE

@chcg chcg changed the title POC scintilla static linkage Build enhancement for scintilla static linkage Mar 25, 2021
…a dynamic one

- moved boostregex out of scintilla dir and adapted build configs therefore
- reverted changes to deps.mak, nmdeps.mak, DepGen.py
@donho
Copy link
Member

donho commented Apr 12, 2021

Thank you @chcg !

squash the commits, if this is now the expected structure

No problem. I can do it.

update the readme to reflect the changes how to build the project

That could be done later.

Could you share your knowledge/experience to change SciLexer.vcxproj from generation of dll to generation of lib? I suppose you did change via Visual Studio interface? As well for adding ARM64 build? It would be great if you could provide the step by step instructions.

I notice one strange behaviour while using the command Start without Debugging in VS:
image

In any configuration after the first Build, if I launch Start without Debugging without launching Start Debugging firstly, Start without Debugging command fails. But after launching Start Debugging, then there's no more error.

@donho donho added the accepted label Apr 12, 2021
@donho
Copy link
Member

donho commented Apr 12, 2021

See https://www.boost.org/development/index.html 14.Apr

2 days more...
I think we can wait 2 days more for its official release :)

@chcg
Copy link
Contributor Author

chcg commented Apr 14, 2021

  • ARM64 was already there in SciLexer.vcxproj
  • Renaming "Debug| ->"Unicode Debug| and "Release| ->"Unicode Release| was done with a text editor and not within VS Studio
  • also adding <ClCompile Include="..\..\boostregex\*.cxx" /> and <ClInclude Include="..\..\boostregex\*.h" /> I did in the text editor
  • adding <WindowsTargetPlatformVersion>10.0.17763.0</WindowsTargetPlatformVersion> and changing the other settings I did in VS settings itself. Were applicable I tried to change a setting for all configurations and platforms to a common value:
    grafik
    grafik
    grafik
    grafik
    grafik
    grafik
    grafik
    grafik
    grafik

Further todo:

  • change the installer @donho will you take care on this?

@donho
Copy link
Member

donho commented Apr 14, 2021

@chcg Thank you for your instructions.

change the installer @donho will you take care on this?

Yes, I'll take care of it.

@donho
Copy link
Member

donho commented Apr 14, 2021

@chcg
Please let me know when you update some files with boost v1.76 release.
( Just checked boost homepage and v1.76 is not released yet.)

@donho
Copy link
Member

donho commented Apr 16, 2021

1.76 RC1 ?
https://www.boost.org/development/index.html
They lied!

@donho
Copy link
Member

donho commented Apr 17, 2021

@chcg
I will merge this PR now. It's not necessary to wait for boost 1.76 release - we could wait for 1 more week.
Please create a new PR if there's any update from boost 1.76.

@donho donho closed this in ab58c8e Apr 17, 2021
@mere-human
Copy link
Contributor

It is nice this finally got checked in!
A first build is a bit slower than usual. But since SciLexer isn't changed often, the incremental build should be as fast as before.
Also, I noticed there are a couple of warnings from SciLexer. Should we suppress them in the project file?
Otherwise works well 👍

@chcg chcg deleted the appveyor_static_linkage branch April 17, 2021 07:03
@chcg
Copy link
Contributor Author

chcg commented Apr 17, 2021

@donho Final release of boost 1.76 is out, no change except of an additional
#include <limits>
which seems not to hurt us, so I won't create a PR for that. I will then add ARM64 support to PluginAdmin list. Ok?

@chcg
Copy link
Contributor Author

chcg commented Apr 17, 2021

@mere-human Slower compared to what? Nmake build? Now boost regex is header only and needs to be also built instead of just linked.

Warnings are there for the boostregex iterators:

c:\program files (x86)\microsoft visual studio\2017\community\vc\tools\msvc\14.16.27023\include\xutility(462): warning C4996: 'std::iterator<std::bidirectional_iterator_tag,char,ptrdiff_t,_Ty *,_Ty &>::iterator_category': warning STL4015: The std::iterator class template (used as a base class to provide typedefs) is deprecated in C++17. (The header is NOT deprecated.) The C++ Standard has never required user-defined iterators to derive from std::iterator. To fix this warning, stop deriving from std::iterator and start providing publicly accessible typedefs named iterator_category, value_type, difference_type, pointer, and reference. Note that value_type is required to be non-const, even for constant iterators. You can define _SILENCE_CXX17_ITERATOR_BASE_CLASS_DEPRECATION_WARNING or _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to acknowledge that you have received this warning. [C:\projects\notepad-plus-plus\scintilla\win32\SciLexer.vcxproj]

See e.g. https://www.fluentcpp.com/2018/05/08/std-iterator-deprecated/ , so we need to define something like:

class UTF8DocumentIterator
{
public:
/** \name std::iterator_traits support */
    using iterator_category = std::bidirectional_iterator_tag;
    using value_type = wchar_t;
    using difference_type = wchar_t;
    using pointer = wchar_t*;
    using reference = wchar_t&;

instead of :

class UTF8DocumentIterator : public std::iterator<std::bidirectional_iterator_tag, wchar_t>

@sasumner
Copy link
Contributor

It is nice this finally got checked in!

I second this sentiment! Nice work, @chcg Christian!
This is a major improvement in the build process.

@chcg Will you work to update BUILD.md since you are most-knowing on this topic?

Note: I will make slight tweaks to this because of the build change:
https://github.com/notepad-plus-plus/notepad-plus-plus/wiki/Testing

@sasumner
Copy link
Contributor

Note: I will make slight tweaks to this because of the build change:
https://github.com/notepad-plus-plus/notepad-plus-plus/wiki/Testing

UPDATE: The testing document has been modified to correspond to the changes made by this PR commit.

@chcg
Copy link
Contributor Author

chcg commented Apr 17, 2021

@sasumner I could create another PR with the BUILD.md update

@donho
Copy link
Member

donho commented Apr 17, 2021

@chcg

release of boost 1.76 is out, no change except of an additional
#include
which seems not to hurt us, so I won't create a PR for that.

Please create a PR for this - the point is to align with v1.76 boost RegExpr release.

I will then add ARM64 support to PluginAdmin list. Ok?

You mean nppPluginList. Yes sure, please.

@mere-human
Copy link
Contributor

@chcg

Slower compared to what? Nmake build? Now boost regex is header only and needs to be also built instead of just linked.

Right. Plus I forgot we had a prebuilt SciLexer.dll anyway.

@chcg chcg mentioned this pull request Apr 17, 2021
@chcg chcg mentioned this pull request Nov 13, 2021
rdipardo referenced this pull request Apr 1, 2022
SCI_LOADLEXERLIBRARY has been removed since Scintilla 5,
and I belive that Scintilla won't support it anymore:
https://sourceforge.net/p/scintilla/bugs/2236/

Close #11451
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants