Update dependency list #2311
Update dependency list #2311
Conversation
|
Hi @andschwa, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
TTYL, MSBOT; |
|
|
There was a problem hiding this comment.
I'm not a core person to PowerShell, but looking over these commits it was hard to find the needle in the haystack -- except in the one pre-existing location where the needle was clearly identified.
If this were my project, I'd lean towards identifying the needle (as the pre-existing comment does).
You can of course take-or-leave this advice.
| @@ -48,7 +58,17 @@ Then execute the following in the terminal: | |||
| > Please note the different libicu package dependency! | |||
| elseif ($IsUbuntu16) { "libicu55" } | ||
| # Setup package dependencies | ||
| # These should match those in the Dockerfiles, but exclude tools like Git, which, and curl | ||
| $dependencies = if ($IsUbuntu) { |
There was a problem hiding this comment.
I wish this block had a similar comment
There was a problem hiding this comment.
I deduplicated it.
There was a problem hiding this comment.
You de-duplicated the note about differing libicu versions?
There was a problem hiding this comment.
No I deduplicated the lists of packages; only libicu differs.
There was a problem hiding this comment.
Oh. Sorry, I just realized you pushed a newer version (perhaps based on my prodding). Yes, the newer version is much clearer, thanks!
| ncurses-base \ | ||
| libunwind \ | ||
| uuid \ | ||
| zlib | ||
| sudo yum install "./$package" | ||
| ;; | ||
| ubuntu) | ||
| case "$VERSION_ID" in | ||
| 14.04) |
There was a problem hiding this comment.
I wish this had a comment like the one above
There was a problem hiding this comment.
Eh this is installing literally the packages that will be listed as dependencies by the package.
There was a problem hiding this comment.
I was experimenting w/ github's review feature. I think that it didn't actually make my comments easier to understand at all :-( -- The comment that I liked is the one calling the reader's attention to the differing versions of libicu.
There was a problem hiding this comment.
I'm experimenting with just dpkg -i followed by apt-get install -f. It's supposed to work to resolve dependencies automatically. (And I don't know why I was yum installing dependencies; that's never been required.) There will probably be ugly error messages I need to hide.
andyleejordan
force-pushed
the
dependencies
branch
from
c92df17 to
6f723a9
Compare
|
So, at least on Ubuntu 16.04, here are the listed depends that are categorized as "required":
On the one hand, these are required in the sense that .NET Core listed them as dependencies, and Ubuntu also requires them for a base system, so there is no harm in specifying them (they're already going to be installed, and PowerShell wouldn't work without them); but it's also redundant. My personal opinion right now is to keep them as dependencies. I see no reason not to except to clean things up slightly (but that clean up would be technically incorrect). Similarly, the packages
are not required in the sense that PowerShell would crash without them. They are necessary for practically anything use the web cmdlets (and we have issues filed due to libcurl not being installed). Perhaps these belong in recommends or suggests instead of depends, I'm not sure. I don't see major harm in depending on them (someone, somewhere, might complain we installed something unnecessarily for their use case, maybe). |
andyleejordan
force-pushed
the
dependencies
branch
4 times, most recently
from
acc8f28 to
1a4adf1
Compare
Now that I finally have a decent list of dependencies (though it may still be too many), I have expanded the dependencies of the package so that users get better error messages. I've removed ca-certificates as that's not *necessarily* required (but needed on Docker to download over HTTPS). This may be true of other packages too.
andyleejordan
force-pushed
the
dependencies
branch
from
1a4adf1 to
b959c8d
Compare
Use `dpkg -i` followed by `apt-get install -f`. Alternatives considered included using `apt install`, which does not work, and copying the PowerShell package to `/var/cache/apt/archives` which is just as messy as this. At least this follows the same steps as `download.sh`.
andyleejordan
force-pushed
the
dependencies
branch
2 times, most recently
from
448330b to
8a24d26
Compare
Again, .NET Core expects users to forcibly link the third party OpenSSL libraries into system directories, which the Homebrew team advises strongly against (and attempts to prevent). This also affects the System.Net.Http library, and results in runtime errors during SSL certificate validation. So instead, we patch what we can, when we can.
|
Got sign off from @vors, merging. |
I've updated the dependency lists in
download.sh,New-UnixPackage, and the Linux installation documentation to match those used in the Dockerfiles.I feel that this probably needs to be pruned; but I am unsure which dependencies we do not need to specify (probably ones listed in "required" for the base distributions, and possibly some that .NET just states as required but is not (like libgcc?)). I would really like the dependency list to be reviewed by others.
This resolves #1937.
Also resolves #2211.