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

Boost appveyor build #9574

Closed
wants to merge 9 commits into from
Closed

Conversation

Uhf7
Copy link
Contributor

@Uhf7 Uhf7 commented Feb 24, 2021

Make boost regex available in Appveyor builds.

There are unnecessary differences between Appveyor builds and Notepad++ release builds:

  • The release builds of Notepad++ contain a SciLexer.dll with improved RegEx behavior
  • The Appveyor builds do not.

This PR intents to overcome this problem.

Edit 2: Since Appveyor has no 1.70 version of boost installed for the current compiler MS VC 2017, I used boost 1.69 here. I believe, that this is better than no boost at all.

appveyor.yml Outdated
@@ -33,9 +33,8 @@ build_script:
- cd "%APPVEYOR_BUILD_FOLDER%"\scintilla\win32
- if "%configuration%"=="Unicode Debug" set scintilla_debug=DEBUG=1
- if "%configuration%"=="Unicode Release" set scintilla_debug=
- if "%archi%" NEQ "" nmake SUPPORT_XP=1 %scintilla_debug% -f scintilla.mak
- if "%archi%" NEQ "" nmake SUPPORT_XP=1 %scintilla_debug% BOOSTPATH=C:\Libraries\boost_1_69_0\ BOOSTREGEXLIBPATH=C:\Libraries\boost_1_69_0\lib32-msvc-14.1\ -f scintilla.mak
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you need to download the boost first. Maybe, use a package from NuGet or some other package manager?
Also, it could be checked out from git repo and built. However, that might slowdown AppVeyor build significantly.

Copy link
Contributor Author

@Uhf7 Uhf7 Feb 24, 2021

Choose a reason for hiding this comment

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

No, I don't. Everything is pre-installed. I'm trying to find out where exactly right now ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will not slow down the Appveyor build.

What I found out yet:

  • The Appveyor doc is correct. For MS2017, only boost 1.69 is available
  • Boost 1.69 works for the 32-bit version.

Next:

  • Make it work for the 64-bit version.

Copy link
Contributor Author

@Uhf7 Uhf7 Feb 24, 2021

Choose a reason for hiding this comment

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

Test 3 revealed, that Boost 1.70 is not available for MS VC 2017.
Edit: Test 3 was crap. Bad parameters.

Copy link
Member

Choose a reason for hiding this comment

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

SUPPORT_XP=1 is not necessary anymore.

@Uhf7
Copy link
Contributor Author

Uhf7 commented Feb 24, 2021

OK, after Test 4 I know, that Boost 1.70 is really not available.

@chcg
Copy link
Contributor

chcg commented Feb 24, 2021

@Uhf7 You could see the installed boost images for appveyor here:
https://www.appveyor.com/docs/windows-images-software/#boost

To my knowledge that are just the sourcecode versions of boost. So just header only components could be used directly. Regex is not of that kind and need to be build, see e.g. https://www.nuget.org/packages/boost_regex-vc141/ for a nuget package.
With some older version such a packages was also part of the scintilla build via the vcxproj file, see https://github.com/notepad-plus-plus/notepad-plus-plus/blob/v7.4/scintilla/win32/packages.config

You could install boost from https://dl.bintray.com/boostorg/release/1.75.0/binaries/ for windows. You could have a look at https://github.com/sergey-shandar/getboost/pull/59/files how to achieve that for appveyor.

@Uhf7
Copy link
Contributor Author

Uhf7 commented Feb 24, 2021

@chcg,
I looked exactly at this page (https://www.appveyor.com/docs/windows-images-software/#boost), my problem here is, that boost 1.69 is pre-installed for MS VC2017, but boost 1.70 (which is used by the Notepad++ SciLexer version) is not. So, what I'm currently trying, is to use 1.69 to create the SciLexer.dll with boost. If this works, we have to evaluate what is bad in 169 compared to 170. If there are no magnificent improvements of 170 compared to 169, then I would say, let's use 169 with Appveyor builds, it's better than no boost regex at all ...

@mere-human
Copy link
Contributor

Maybe, it is about time to upgrade Notepad++ to be built with MSVC 2019.
I've wanted that for some time🙂

@ArkadiuszMichalski
Copy link
Contributor

There were some changes:
https://www.boost.org/users/history/version_1_70_0.html

@Uhf7
Copy link
Contributor Author

Uhf7 commented Feb 24, 2021

@ArkadiuszMichalski,

too many lines in that, I cannot read it today, and, honestly, I'm not going to read it tomorrow. If you have a certain point in this list, which is important to you, please name it.

The question here is, do we believe, that a 1.69 regex boosted version of SciLexer.dll will allow users to test certain issues closer to reality than with a not-boosted SciLexer.dll version. I believe, that version 1.69 is good enough...

And, of course, @mere-human, if there would be an administrative decision to use MS VC 2019 from now on, then we could use the 1.73 version of boost regex with no Appveyor constraints.

@sasumner
Copy link
Contributor

@ArkadiuszMichalski

There were some changes:
https://www.boost.org/users/history/version_1_70_0.html

Curious, that history says nothing about the regex part of the library -- does that mean there were no changes from 1.69 to 1.70? Hmm, looking back a few links 1_69_0, 1_68_0, 1_67_0, no mention of regex there either. I guess I don't know how to read Boost history docs correctly?

@mere-human

Maybe, it is about time to upgrade Notepad++ to be built with MSVC 2019.
I've wanted that for some time

My secret: I've NEVER had MSVC 2017; always used MSVC 2019 on N++...with the slight inconvenience doing that causes.

@Uhf7

I believe, that version 1.69 is good enough...

I tend to agree.

@donho donho self-assigned this Feb 25, 2021
@Uhf7
Copy link
Contributor Author

Uhf7 commented Feb 25, 2021

@ArkadiuszMichalski,

sorry for complaining about the long list, this is of course the thing which should be checked. Today, it looks much shorter and it seems indeed not to contain anything about regex.

I compared the regex source code of versions 1.69 and 1.70 too, there is no more difference than a new #define BOOST_REGEX_NO_EXTERNAL_TEMPLATES for Cygwin systems in config.hpp. All other regex files are the same. I didn't check all the dependencies within boost, but there seems no intentional change of regex behavior between this two versions.

@ArkadiuszMichalski
Copy link
Contributor

Unfortunately, Notepad ++ still lacks regex tests, it would be a lot easier with them. Function List test passed on this version? Better this than nothing.
One more question for formalities. Will build (appveyor) .dll and release .dll use the same or different version of Boost in future? If Boost doesn't actually change that much from version to version, then maybe different versions won't be a reason for unnecessary bug reports.

@sasumner
Copy link
Contributor

Now that @donho has joined this thread... The last time this kind of thing came up, Don mentioned he would "look into static linking" of Scintilla and thus by inference Boost.

@donho
Copy link
Member

donho commented Feb 25, 2021

@ArkadiuszMichalski

Function List test passed on this version?

Yes, all tests passed with new ScinLexer.dll v4.4.6.

Unfortunately, Notepad ++ still lacks regex tests, it would be a lot easier with them.

We can add regex test via Function List by using different user defined language.

@sasumner

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

Yes sure. I will check it after releasing of next version.

@mere-human

Maybe, it is about time to upgrade Notepad++ to be built with MSVC 2019.

It's on my radar.

@sasumner
Copy link
Contributor

So the summary is, now that this is accepted, that future appveyor builds of SciLexer will contain Boost 1.69.

SUPPORT_XP was retained, is that relevant, or was a good opportunity to remove it missed?

@donho
Copy link
Member

donho commented Feb 25, 2021

@sasumner

So the summary is, now that this is accepted, that future appveyor builds of SciLexer will contain Boost 1.69.

Correct.

SUPPORT_XP was retained, is that relevant, or was a good opportunity to remove it missed?

I create a new issue here: #9581 and assign to @chcg.
It could be done in this PR.

donho pushed a commit to donho/notepad-plus-plus that referenced this pull request Feb 27, 2021
Currently in Appveyor build, we download SciLixer.dll from the latest release for Unit tests (of function list especially).
In PR notepad-plus-plus#9574 the boost is included in Appveyor build.

In this PR:
- Remove the download last release part from Appveyor script and use the generated SciLexer.dll
- Remove SUPPORT_XP from the script

Fix notepad-plus-plus#9581, close notepad-plus-plus#9591
donho pushed a commit that referenced this pull request Feb 27, 2021
Currently in Appveyor build, we download SciLixer.dll from the latest release for Unit tests (of function list especially).
In PR #9574 the boost is included in Appveyor build.

In this PR:
- Remove the download last release part from Appveyor script and use the generated SciLexer.dll
- Remove SUPPORT_XP from the script

Fix #9581, close #9591
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

6 participants