feat(build.bat): customize PLATFORM_TOOLSET settings by lotem · Pull Request #112 · rime/weasel

lotem · 2018-02-20T16:02:02Z

Changes:

introduces PLATFORM_TOOLSET variable (defaults to v140_xp)
renames BOOST_HOME to BOOST_ROOT (to be consistent with librime/build.bat)

Prcuvu

Setting platform toolset to environmental variable seems erroneous, both on AppVeyor and my local build environment. CMake may be a better solution.

lotem · 2018-02-21T00:51:03Z

The check failed because I did not set the environment variable PLATFORM_TOOLSET in appveyor.yml. When I did to my local build, it worked.

The same environment variable also worked for several projects in librime/thirdparty/src/ with this change rime/librime#178 . No *.props file was needed to introduce the variable to project files.

However, I did not expect user to set the environment variable, but to define the macro in weasel.props.
Somehow the default value set in weasel[-appveyor].props did not take effect.
Similarly, I expect BOOST_ROOT to be set in weasel.props in most cases, and set the environment variable only as a convenient way to override the default value.

nameoverflow

So add PLATFORM_TOOLSET in appveyor.yml to pass CI check? Will it work if create a weasel.props in appveyor.install.bat ?

Prcuvu · 2018-02-21T06:16:27Z

It turns out that weasel.props must be imported before using any of its macros.

Prcuvu

Looks good now.

lotem · 2018-02-21T06:51:30Z

-set boost_libs=--with-date_time --with-filesystem --with-locale --with-regex --with-signals --with-system --with-thread
-cd %BOOST_ROOT%
+
+set boost_build_flags=toolset=msvc-%VisualStudioVersion%^


Should not use %VisualStudioVersion% here.
The number is platform toolset, not Visual Studio version.
For example msvc-14.1 for Visual Studio 15 2017.

https://stackoverflow.com/questions/42730478/version-numbers-for-visual-studio-2017-boost-and-cmake/42738533#42738533

lotem · 2018-02-21T07:02:50Z

Well, it's more complicated than what I thought.
It's great the properties file works as expected now with @Prcuvu's fix.
We might also need to set environment variables since
build.bat boost requires BOOST_ROOT and BJAM_TOOLSET set in CMD.
User have to set the environment variables manually at first run. Maybe it's a good idea to generate weasel.props to avoid typing those values twice.
It'll be nice not having to set environment variables afterwards, working in Visual Studio without command line.

Prcuvu · 2018-02-21T07:46:02Z

Typing toolset version in both build script and property sheet is inevitable since we do not build boost in Visual Studio IDE. It is a one-time job after all.

nameoverflow

LGTM.

lotem · 2018-02-21T09:04:10Z

Wait... I have more code to commit.

I would like to revert a part of ebf8575
Sorry.

I think it's not correct to auto-detect BJAM_TOOLSET based on VisualStudioVersion. It's ugly and vulnerable to rewrite the version number to platform toolset number. Eventually we have to maintain a mapping table of versions in the script. No.
Just set a default value and let user override it with an environment variable if they wish.

I'm working on converting .template->.vsprops. With that user only have to set environment variable once.

Prcuvu · 2018-02-21T09:09:54Z

@lotem I would like to leave the job to users. If you come up with some better approach, force update is welcome.

…defaults

…template

lotem · 2018-02-22T16:02:50Z

Can we sync submodule librime to tag rime-1.2.10 ?
With latest librime, environment settings in weasel\env.bat works for both weasel\build.bat and librime\build.bat.

Prcuvu · 2018-02-22T16:19:27Z

Currently AppVeyor CI downloads librime's CI artifact of latest commit, which is ugly and problematic.
I'm planning to build librime in weasel's CI install stage and cache built binaries.

lotem · 2018-02-22T16:41:49Z

@Prcuvu

is ugly and problematic

Why do you think it's ugly? Can you share more details on encountered problems with this approach?

To the contrary, I'm thinking about splitting librime into smaller projects to save CI build time by reusing artifacts from dependency projects.

build librime in weasel's CI install stage and cache built binaries.

This isn't trivial. Each build has to be tagged with librime version (commit hash) and compared to the librime version referenced in current build.

Are the binaries shared among build machines?

Prcuvu · 2018-02-22T18:20:13Z

I say it's ugly because I have to modify appveyor.install.bat and add new artifact download URL every time I upgrade submodule version. Modification of appveyor.install.bat also triggers boost rebuild.
If we always download artifact from latest commit, it will break weasel if librime of that commit happens to have a bug.

I would like to add a librime build cache, whose dependency tracks submodule change (if possible), into appveyor.yml.

Prcuvu · 2018-02-24T13:20:07Z

Well, reusing artifacts is a more economic approach. I will just specify artifact URL every time librime changes.

Feel free to merge when you think it is ready.

lotem · 2018-02-24T14:50:13Z

There is one more thing:
librime/build/include is missing in local build, and causes weasel/build.bat to fail.
Though the appveyor script handles this, it's still broken for user.
For that I'll update submodule librime to include this change
rime/librime@06c9e86
(Testing...)

feat(build.bat): customize PLATFORM_TOOLSET settings

5a1bb45

lotem requested review from Prcuvu and nameoverflow February 20, 2018 16:02

lotem mentioned this pull request Feb 20, 2018

feat(build.bat): customize build settings via environment variables rime/librime#178

Merged

Prcuvu reviewed Feb 20, 2018

View reviewed changes

nameoverflow reviewed Feb 21, 2018

View reviewed changes

Fix include order in project files.

3c6d76c

Prcuvu approved these changes Feb 21, 2018

View reviewed changes

lotem commented Feb 21, 2018

View reviewed changes

nameoverflow approved these changes Feb 21, 2018

View reviewed changes

chore(boost.bat): improve detection for :build_boost

ebf8575

Prcuvu force-pushed the build branch from 1a0ef85 to ebf8575 Compare February 21, 2018 09:00

Update INSTALL.md

66787f0

lotem and others added 4 commits February 21, 2018 17:57

chore(build.bat): let user set environment variables while providing …

709e697

…defaults

feat(build.bat): generate weasel.props by interpolating variables in …

796d202

…template

chore(appveyor): use auto-generated weasel.props

3edd85e

fix(render.js): use ADO stream to handle UTF-8 encoding

75999ec

Prcuvu force-pushed the build branch from 77d07e5 to 75999ec Compare February 21, 2018 12:20

lotem added 2 commits February 22, 2018 00:56

chore(render.js): display interpolated environment variables

0f12562

chore(build.bat): fix typo [ci skip]

323b5bb

chore(submodule): upgrade to librime 1.2.10

31367e5

chore(appveyor.install.bat): download librime 1.2.10

0a2dfb6

lotem closed this Feb 25, 2018

lotem deleted the build branch February 25, 2018 15:39

Conversation

lotem commented Feb 20, 2018

Uh oh!

Prcuvu left a comment

Choose a reason for hiding this comment

Uh oh!

lotem commented Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nameoverflow left a comment

Choose a reason for hiding this comment

Uh oh!

Prcuvu commented Feb 21, 2018

Uh oh!

Prcuvu left a comment

Choose a reason for hiding this comment

Uh oh!

lotem Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lotem commented Feb 21, 2018

Uh oh!

Prcuvu commented Feb 21, 2018

Uh oh!

nameoverflow left a comment

Choose a reason for hiding this comment

Uh oh!

lotem commented Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Prcuvu commented Feb 21, 2018

Uh oh!

lotem commented Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Prcuvu commented Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lotem commented Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Prcuvu commented Feb 22, 2018

Uh oh!

Prcuvu commented Feb 24, 2018

Uh oh!

lotem commented Feb 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lotem commented Feb 21, 2018 •

edited

Loading

lotem Feb 21, 2018 •

edited

Loading

lotem commented Feb 21, 2018 •

edited

Loading

lotem commented Feb 22, 2018 •

edited

Loading

Prcuvu commented Feb 22, 2018 •

edited

Loading

lotem commented Feb 22, 2018 •

edited

Loading