Add windows-terminal
#21913
Add windows-terminal
#21913
Conversation
|
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
There was a problem hiding this comment.
This is good to go, insofar as it works to install the pre-built Windows Terminal application.
I think this will be very handy to make the lives of Windows developers (and those who have to use Windows) easier.
As a self-contained application, the binary repackaging shouldn't cause any conflicts or maintenance burden.
|
|
|
ping @conda-forge/core - you may have to weigh in on this one... |
|
pretty cool! |
| if %ERRORLEVEL% neq 0 exit 1 | ||
| 7za x -oterminal .\CascadiaPackage_{{ version }}1.0_x64.msix | ||
| if %ERRORLEVEL% neq 0 exit 1 | ||
| xcopy terminal "%PREFIX%\windows-terminal" /E /I /F /B /Y |
There was a problem hiding this comment.
Is this creating a windows-terminal directory in the PREFIX root? I think we might want to use a different containing directory, but I am not sure which one is best in Windows.
There was a problem hiding this comment.
My understanding is that scripts/ is mostly used for entry-points, so having wt.cmd in scripts/ makes sense as it's the entry-point for the Windows Terminal application.
There was a problem hiding this comment.
I'm not sure it makes sense putting the applications themselves under scripts/, but I'm also not sure there's any conda-forge convention for where to put applications (particularly on
Windows)
There was a problem hiding this comment.
If we look at what nodejs does, it puts node-modules in the top-level PREFIX, but it also puts the entry-points in PREFIX as well:
❯ ls C:\mambaforge\envs\node
Directory: C:\mambaforge\envs\node
Mode LastWriteTime Length Name
---- ------------- ------ ----
d---- 13/02/2023 21:08 conda-meta
d---- 13/02/2023 21:08 node_modules
-a--- 05/11/2022 02:26 65491576 node.exe
-a--- 05/11/2022 00:48 1365 npm
-a--- 05/11/2022 00:48 483 npm.cmd
-a--- 05/11/2022 00:48 1567 npx
-a--- 05/11/2022 00:48 539 npx.cmd
It's likely that the node.exe binary expects to find the node-modules folder alongside it so that constrains where you could place the executable.
IMHO it would probably be cleaner to put the .cmd files in scripts/, but if the actual binaries are in PREFIX anyway, it probably doesn't have any benefit.
There was a problem hiding this comment.
I'm fairly sure that R used to be installed into PREFIX/R but installing the latest version it appears to be installed into PREFIX/lib/R
❯ ls C:\mambaforge\envs\rdev\lib\R
Directory: C:\mambaforge\envs\rdev\lib\R
Mode LastWriteTime Length Name
---- ------------- ------ ----
d---- 13/02/2023 21:11 bin
d---- 13/02/2023 21:11 doc
d---- 13/02/2023 21:11 etc
d---- 13/02/2023 21:11 include
d---- 13/02/2023 21:11 library
d---- 13/02/2023 21:11 modules
d---- 13/02/2023 21:11 share
d---- 13/02/2023 21:11 src
d---- 13/02/2023 21:11 Tcl
d---- 13/02/2023 21:11 tests
-a--- 19/12/2022 19:45 27421 CHANGES
-a--- 19/12/2022 19:45 18011 COPYING
-a--- 19/12/2022 19:45 4132 README
-a--- 19/12/2022 19:45 8676 README.R-4.1.3
...with entry-points installed into PREFIX/scripts
❯ ls C:\mambaforge\envs\rdev/Scripts
Directory: C:\mambaforge\envs\rdev\Scripts
Mode LastWriteTime Length Name
---- ------------- ------ ----
-a--- 19/12/2022 19:24 22016 open.exe
-a--- 19/12/2022 19:24 22016 R.exe
-a--- 19/12/2022 19:24 22016 Rcmd.exe
-a--- 19/12/2022 19:24 22016 Rfe.exe
-a--- 19/12/2022 19:24 22016 Rgui.exe
-a--- 19/12/2022 19:24 22016 Rscript.exe
-a--- 19/12/2022 19:24 22016 RSetReg.exe
-a--- 19/12/2022 19:24 22016 Rterm.exe
There was a problem hiding this comment.
I'm not sure installing an application under lib makes all that much sense, but it also probably doesn't matter - it's transparent to the user calling R.exe in the PREFIX/Scripts folder.
There was a problem hiding this comment.
On Windows, the dotnet language installs into PREFIX/dotnet but, since I was the one who made that arbitrary decision, it can't really be used as a precedent here 😄
There was a problem hiding this comment.
At the end of the day, I'm happy to install it wherever you think makes most sense.
Following R that would be in PREFIX/lib/windows-terminal but following nodejs that would be in PREFIX/windows-terminal.
I think the status-quo (nodejs convention) makes the most sense, but I also don't really mind and am happy to follow any convention (that doesn't require changes to the application)
| @@ -0,0 +1 @@ | |||
| @"%~dp0..\windows-terminal\wt.exe" %* | |||
There was a problem hiding this comment.
Should this file be named .cmd or .bat? I think there are behaviour differences (see https://stackoverflow.com/questions/148968/windows-batch-files-bat-vs-cmd), but most scripts in conda-forge are .bat. Reading that link though it looks like .cmd is the modern recommendation? 🤔
There was a problem hiding this comment.
If we move everything under Scripts we might not need this.
There was a problem hiding this comment.
I view .bat and .cmd as pretty interchangeable. I've never encountered a situation where it mattered though the SO you linked to is pretty informative! It does seem that .cmd is a bit stricter and generally preferred.
My general adhoc convention is to call scripts .bat and reserve .cmd for files which just set up an environment before calling an external binary.
Since the script doesn't use any of the mentioned commands where there is a difference in errorlevel handling I think it's a moot point. If you'd prefer it be a .bat file I can make that change.
There was a problem hiding this comment.
If we move everything under
Scriptswe might not need this.
If the application was under scripts/windows-terminal you'd still need a driver script in scripts/ to call it without having to resort to the full pathname of the executable.
|
Thanks for this! I wonder how tricky is to build from source instead of repackaging, given that it's MIT licensed? There are a few overlinking warnings that are now considered errors, so we might need adjustments in the runtime dependencies. |
|
Thanks for the review @jaimergp! I'll try to respond to the other points tonight... |
|
Sorry for the delay, my OS time is fairly limited these days :( ...which unfortunately means I won't have time in the foreseeable future to investigate building from source. I'm not a Windows/C++ dev so it would likely be a huge investment of time with a probable outcome of failure. I think in this case the downsides of binary repacking mostly don't apply so the benefit of being able to I absolutely agree that in a perfect world it would be better to be built from source, but in reality that's not going to happen until someone with the requisite skills and motivation comes along to volunteer their time. Until such time, Windows/mamba users would greatly benefit from being able to install a usable terminal with their chosen package manager. At least on the Windows side, there is plenty of precedent for allowing binary repackages for applications as it's better than the alternative of |
|
Yea I get that, nw. Binary repackaging can be considered, but right now there are a few warnings that I am not sure about. They might be harmless but I don't have the knowledge to assess :( |
I'll look into those now. I couldn't see what you were referring to from a quick look on my phone but I'm at my desktop now so will investigate properly and get back to you... |
Probably referring to: |
|
The unique missing dlls: |
|
I'm slightly confused about IIUC |
Co-authored-by: jakirkham <jakirkham@gmail.com>
|
Looks like adding the C compiler fixed some of the issues. Would try adding the C++ compiler to see if that fixes the rest of the issues |
Co-authored-by: jakirkham <jakirkham@gmail.com>
|
Thanks for the help @jakirkham! It looks like its' still complaining about: |
|
Checking one of the dlls in my dev env: |
recipes/windows-terminal/meta.yaml
Outdated
| run: | ||
| - ucrt |
There was a problem hiding this comment.
Don't think this should be needed as the compilers will pull this in.
Possibly the issue is we have the wrong version of compiler. Would take a look at the different vs20XY_win-64 packages (like vs2015_win-64, vs2017_win-64, and vs2019_win-64) to see if one of them is a better fit.
Once we figure that out, one can set the compiler versions used by the recipe.
There was a problem hiding this comment.
I was trying the whack-a-mole technique 😬
There was a problem hiding this comment.
Yeah no worries. Know how this goes. Wanted to save you from doing that exploration in the dark 😉
I'm not sure why |
That explains it I guess |
|
It appears to still be complaining about missing dlls with |
|
No joy. Solver error this time. |
|
Well, that worked 😬 I'm not sure what else I could try to squash the overlinking errors but am happy to try if there are any suggestions, or happy for anyone else to take a crack and push to my branch. At this point, I'm tending towards merging with the overlinking errors disabled as the application works (for me) despite them. If it does in fact cause users issues it might be easier to debug in the feedstock with a failing test case (as and when that problem arises) |
|
This is vendoring a lot of DLLs like |
I think they're all part of a redistributable package but I couldn't find out which and trying all the different vs compiler packages didn't work :( It would be very handy if there were a tool to search which packages provide which files. |
Manually inspecting the |
|
In a Windows Container I installed the latest VC redist from: ...and diffed the dlls in the In [4]: dlls2 - dlls1
Out[4]:
{'concrt140.dll',
'mfc140.dll',
'mfc140chs.dll',
'mfc140cht.dll',
'mfc140deu.dll',
'mfc140enu.dll',
'mfc140esn.dll',
'mfc140fra.dll',
'mfc140ita.dll',
'mfc140jpn.dll',
'mfc140kor.dll',
'mfc140rus.dll',
'mfc140u.dll',
'mfcm140.dll',
'mfcm140u.dll',
'msvcp140.dll',
'msvcp140_1.dll',
'msvcp140_2.dll',
'msvcp140_atomic_wait.dll',
'msvcp140_codecvt_ids.dll',
'vcamp140.dll',
'vccorlib140.dll',
'vcomp140.dll',
'vcruntime140.dll',
'vcruntime140_1.dll'}So, I'm still not sure where the dlls are all coming from :/ |
|
I haven't found a way to make the overlink checker happy, and if there are concerns about redistributing the dlls from the msixbundle, I might have to put this one in the too-hard / not-enough-time-to-investigate-further basket. This PR will still be in the history if anyone wants to copy or pick it up... |
|
Thanks @dhirschfeld! |
Checklist
url) rather than a repo (e.g.git_url) is used in your recipe (see here for more details).