Add ability to use MSVS 2019 and pre-release versions of MSVS. Fix other nits.
AbandonedPublic

Authored by vonreth on Mar 21 2019, 3:06 AM.

Details

Reviewers
rboylan
Summary

BUG: 405662

  1. Add optional argument localDev to the primary powershell script to allow using a locally modified version of craft. Intended to facilitate development.
  2. Correct some strings using \ which caused python to fail when loading the script.

I'm not sure how they lasted so long as they are. Python 3.7 on Win10 produced an error when
trying to load them.

  1. Various typos in messages to user corrected. Some messages reworded for clarity.
  2. The search for pre-release versions of MSVS is automatic, occurring only if the regular

search finds nothing. A note is printed to the log if a pre-release is used.

  1. MS Visual Studio 2019 is the default compiler.
  2. Since I am working with a pre-release version of MSVS 2019 it's possible things will be different on final release.
  3. The version I entered for MSVC Platform Toolset is a guess; I don't even know what the toolset is.

I also don't really know what the initial parm block in install_craft.ps1 is about; that didn't stop me from adding an argument in what seemed the right style.

Test Plan

Ideally, testing with the release version of VS 2019 as well as with an older version would be good.

Diff Detail

Repository
R138 Craft
Lint
Lint Skipped
Unit
Unit Tests Skipped
rboylan requested review of this revision.Mar 21 2019, 3:06 AM
rboylan created this revision.
vonreth requested changes to this revision.Mar 21 2019, 8:25 AM
vonreth added a subscriber: vonreth.

Thx!

bin/CraftCompiler.py
310

We don't support xp so ๐Ÿคทโ€โ™€๏ธ

blueprints/libs/runtime/runtime.py
55

I think that file was already changed so the patch needs an update,
also could you unify the check with msvc2017?

setup/CraftBootstrap.py
172

I really don't want to encourage people to use 2019 yet.
So we could keep that in but maybe ide the option by default unless a "pro/dev" mode was activated?

This revision now requires changes to proceed.Mar 21 2019, 8:25 AM
vonreth added inline comments.Mar 21 2019, 8:27 AM
setup/install_craft.ps1
137

remove the debug string

rboylan added a comment.EditedMar 24 2019, 8:33 PM

Thank you for your rapid response.

First of all, are there any instructions/guidelines for how to use this site? It kind of looks as if the patch can be edited in place, but I have other changes that will require a new diff anyway.

The code in the patch always fails as outlined in my associated bug report (only in localDev mode) because it copies the craft directory including .git. .git included read only files, so that the step that deletes the craft-master directory after most things are done fails (the problem was not, as I originally thought, that I had some application open on the directory or file). I fixed that by excluding .git from the copy.

This in turn revealed more problems. Something, I think either of both of craft-blueprints-kde.py or craft-core.py, is causing craft to be refetched from upstream.

rdboylan@CVRI-FahyDev ~/Documents/src/craft
$ grep -R anongit.kde.org .
./blueprints/craft/craft-blueprints-kde/craft-blueprints-kde.py:            self.svnTargets[ver] = f"[git]git://anongit.kde.org/craft-blueprints-kde|{ver}|"
./blueprints/craft/craft-core/craft-core.py:            self.svnTargets[ver] = f"git://anongit.kde.org/craft|{ver}|"

I think this is where it came from:

==============================================
*** Handling package: craft/craft-core, action: all ***
*** Action: fetch-binary for craft/craft-core ***
*** craft/craft-core not found in cache ***
*** Action: fetch for craft/craft-core ***
executing command: C:\Program Files\Git\cmd\git.exe clone git://anongit.kde.org/craft C:\Users\rdboylan\Documents\src\CraftRoot2\craft
Cloning into 'C:\Users\rdboylan\Documents\src\CraftRoot2\craft'...

So my local changes to CraftCompiler.py and other code are not used in the final steps, and, with the fix for the .git directory, I get to:

*** Craft all succeeded: craft/craft-blueprints-kde after 21 seconds ***


Execute: C:\Program Files (x86)\Microsoft Visual Studio\Shared\Python37_64\python.exe C:\Users\rdboylan\Documents\src\CraftRoot2\craft\bin\craft.py dev-utils/snoretoast
Unknown MSVC Compiler msvc2019_64
'msvc2019'
Traceback (most recent call last):
  File "C:\Users\rdboylan\Documents\src\CraftRoot2\craft\bin\craft.py", line 250, in <module>
    success = main()
  File "C:\Users\rdboylan\Documents\src\CraftRoot2\craft\bin\craft.py", line 179, in main
    helper.setupEnvironment()
  File "C:\Users\rdboylan\Documents\src\CraftRoot2\craft\bin\CraftSetupHelper.py", line 288, in setupEnvironment
    os.environ = self.getEnv()
  File "C:\Users\rdboylan\Documents\src\CraftRoot2\craft\bin\CraftSetupHelper.py", line 216, in getEnv
    return SetupHelper.getMSVCEnv(CraftCore.compiler.getInternalVersion(),
  File "C:\Users\rdboylan\Documents\src\CraftRoot2\craft\bin\CraftCompiler.py", line 297, in getInternalVersion
    return versions[c]
KeyError: 'msvc2019'

So I need some way of overriding that if localDev has been set. I am not sure exactly where they best place to do that is, nor do I know how to get the info that localDev is set to that spot of code.
Any help?

setup/install_craft.ps1
137

Which string are you referring to?

rboylan edited the summary of this revision. (Show Details)Mar 24 2019, 10:18 PM

To manage this review you can use arcanist (craft arcanist).
https://secure.phabricator.com/book/phabricator/article/arcanist_diff/

I think your localdev mode is useful to test the setup script, but for the blueprints etc its better to manually modify the etc/CraftSettings.ini, call craft --destroy-craft-root and thus start fresh.
In the end the setup script is meant to help newcomers with ther first time setup.
Advanced user can then also modify the location where the blueprints are looked up so a shared blueprint repository is possible.

setup/install_craft.ps1
137

write-host localdev
which is always printed.

rboylan updated this revision to Diff 54884.Mar 26 2019, 7:18 PM

I think this responds to most of the first round comments.
VS2017 is now the default choice, although I have not hidden VS2019 as you suggested.
The other oddity I noticed is that the Qt version is lower in my version than yours.
The diff is against 738d01f314235d89691d20f78b0cdd689f059779 refs/remotes/origin/master
with
origin https://github.com/KDE/craft.git (fetch)

The problems with the Qt version suggest the patch for runtime.py might still not be against the right base version of the file.

rboylan marked 4 inline comments as done.Mar 26 2019, 7:22 PM

Also, I tested with VS2017, and everything seemed to work.
The current patch set reflects changes made since that test.

setup/CraftBootstrap.py
172

How do we hide an alternative? Would it be enough to leave 2017 as the default choice?

If they don't have VS2019 on their system they won't get very far even if they select it.

I'm new to PowerShell. Although the script clearly has parameters and options, it's not clear to me how to invoke it with options. So I've just been changing the default arguments in the file. Is it as easy as
iex "install_craft.ps1" --localDev "somePath"
or maybe -localDev is the powershell style.
?

But others working on craft must have run into this problem of wanting to use their own code rather than the canonical code. How did they deal with it?

setup/install_craft.ps1
137

Oh, probably the line above .... Yes, I already killed that locally.

vonreth added inline comments.Mar 29 2019, 11:09 PM
etc/CraftCoreSettings.ini
6 โ†—(On Diff #54884)

Don't touch this

setup/CraftBootstrap.py
172

Its a single dash and yes powershell is "special"

176

how about os.environ.get("CRAFT_PRO_SETUP", "0") != "0"
As soon as people see 2019 they will try it and I'd like to wait a month and test stuff myself before I promote it.

Could you split this change?
I'm willing to add the 2019 support but the setup script should not mention it yet.

vonreth commandeered this revision.Apr 17 2019, 8:36 AM
vonreth edited reviewers, added: rboylan; removed: vonreth.

merged

vonreth abandoned this revision.Apr 17 2019, 8:36 AM
vonreth marked an inline comment as done.

Is there something I still need to do here? I was on vacation.

I can't tell what the status of this is.

Note that I have never got it to run successfully with MSVS 2019. I suspect this was for unrelated reasons, namely the reasons I had trouble getting craft to work with VS 2017 (which it now does--thanks for your help).