create script to install latest powershell from repositories #3608
Conversation
|
@DarwinJS, |
|
|
||
| lowercase(){ | ||
| echo "$1" | sed "y/ABCDEFGHIJKLMNOPQRSTUVWXYZ/abcdefghijklmnopqrstuvwxyz/" | ||
| } |
There was a problem hiding this comment.
I think this can be more easily done as
echo "$*" | tr [A-Z] [a-z]
There was a problem hiding this comment.
@JamesWTruher - this came over from download.sh. Do you know if "tr" universally available on al platform targets? The methods in the script will have to favor universal availability of the tools used over elegant implementation.
There was a problem hiding this comment.
Ok did some research and it seems like anything in the posix command set should be considered universal and "tr" is on the list: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/contents.html
tools/install-powershell.sh
Outdated
| OS=osx | ||
| DistroBasedOn=osx | ||
| else | ||
| OS=`uname` |
There was a problem hiding this comment.
could the rest of these comparisons all be lower case comparisons? is there a need to rerun uname?
| elif [ "${OS}" == "Linux" ] ; then | ||
| if [ -f /etc/redhat-release ] ; then | ||
| DistroBasedOn='redhat' | ||
| DIST=`cat /etc/redhat-release |sed s/\ release.*//` |
There was a problem hiding this comment.
I think that cat is unneeded. sed and grep all take a file so this could just be
DIST=$(sed /s\ release.*// /etc/redhat-release)
There was a problem hiding this comment.
This is easier to read to me
tools/installpsh-debian.sh
Outdated
|
|
||
| #Verify The Installer Choice (for direct runs of this script) | ||
| lowercase(){ | ||
| echo "$1" | sed "y/ABCDEFGHIJKLMNOPQRSTUVWXYZ/abcdefghijklmnopqrstuvwxyz/" |
There was a problem hiding this comment.
echo "$1" | sed "y/ABCDEFGHIJKLMNOPQRSTUVWXYZ/abcdefghijklmnopqrstuvwxyz/" [](start = 4, length = 74)
echo "$1" | tr [A-Z] [a-z]
|
|
||
| echo | ||
| echo "*** Installing PowerShell Core for $DistroBasedOn..." | ||
| if ! hash curl 2>/dev/null; then |
There was a problem hiding this comment.
hash [](start = 5, length = 4)
i know this works, but would which be a better choice?
There was a problem hiding this comment.
Not sure - is "which" available on all platform targets including OSX? I will tend to favor what is working over making changes to something that in theory is a better method.
| echo "curl not found, installing..." | ||
| $SUDO apt-get install -y curl | ||
| fi | ||
| release=`curl https://api.github.com/repos/powershell/powershell/releases/latest | sed '/tag_name/!d' | sed s/\"tag_name\"://g | sed s/\"//g | sed s/v//g | sed s/,//g | sed s/\ //g` |
There was a problem hiding this comment.
curl https://api.github.com/repos/powershell/powershell/releases/latest | sed '/tag_name/!d' | sed s/"tag_name"://g | sed s/"//g | sed s/v//g | sed s/,//g | sed s/\ //g` [](start = 9, length = 172)
there's all sorts of ways to do this. here's an alternative:
curl -q [https://api.github.com/repos/powershell/powershell/releases/latest](https://api.github.com/repos/powershell/powershell/releases/latest) 2>/dev/null | awk ' /tag_name/ { print $2 }' | sed "s/[\",]//g"
There was a problem hiding this comment.
I'm sure there are a lot of ways any given thing could be done - really looking for the smallest possible toolset dependencies to get the practical result.
tools/install-powershell.sh
Outdated
| KERNEL=`uname -r` | ||
| MACH=`uname -m` | ||
|
|
||
| if [ "{$OS}" == "windowsnt" ]; then |
There was a problem hiding this comment.
"{ [](start = 5, length = 2)
this should be "${OS}" I think
tools/install-powershell.sh
Outdated
| if [ "{$OS}" == "windowsnt" ]; then | ||
| OS=windows | ||
| DistroBasedOn=windows | ||
| elif [ "{$OS}" == "darwin" ]; then |
There was a problem hiding this comment.
{$OS} [](start = 8, length = 5)
"${OS}"
|
|
||
| echo | ||
| echo "*** Installing PowerShell Core for $DistroBasedOn..." | ||
| if ! hash curl 2>/dev/null; then |
There was a problem hiding this comment.
curl [](start = 10, length = 4)
should you also check for wget?
There was a problem hiding this comment.
In my googling on it seemed that wget is pretty universal - this was also born out by the original download.sh. But I like the idea, because if someone replicates the repo or copies the script set manually - that is a scenario where I can't assume wget was used to pull the script in the first place.
tools/installpsh-osx.sh
Outdated
|
|
||
| #Verify The Installer Choice (for direct runs of this script) | ||
| lowercase(){ | ||
| echo "$1" | sed "y/ABCDEFGHIJKLMNOPQRSTUVWXYZ/abcdefghijklmnopqrstuvwxyz/" |
There was a problem hiding this comment.
sed [](start = 16, length = 3)
tr
tools/installpsh-osx.sh
Outdated
| lowercase(){ | ||
| echo "$1" | sed "y/ABCDEFGHIJKLMNOPQRSTUVWXYZ/abcdefghijklmnopqrstuvwxyz/" | ||
| } | ||
| if [ "{$OS}" == "windowsnt" ]; then |
tools/installpsh-osx.sh
Outdated
| if [ "{$OS}" == "windowsnt" ]; then | ||
| OS=windows | ||
| DistroBasedOn=windows | ||
| elif [ "{$OS}" == "darwin" ]; then |
There was a problem hiding this comment.
$OS [](start = 9, length = 3)
"${OS}"
tools/installpsh-suse.sh
Outdated
| gitreposcriptroot="https://raw.githubusercontent.com/$gitreposubpath/tools" | ||
| thisinstallerdistro=suse | ||
| repobased=true | ||
| gitscriptname="installpsh-redhat.psh" |
There was a problem hiding this comment.
redhat [](start = 26, length = 6)
suse?
There was a problem hiding this comment.
Good catch - thanks!
tools/install-powershell.readme.md
Outdated
| @@ -0,0 +1,56 @@ | |||
|
|
|||
| ## Features of install-powershell.sh | |||
There was a problem hiding this comment.
There are syntax errors in the markdown. Can you please enable these files in this test and fix the syntax errors)
There was a problem hiding this comment.
@TravisEz13 - I'm not sure what you mean. "##" is level two heading.
I also don't know what you mean by "Can you please enable these files in this test..." - how would I do this?
Please point me to a markdown validator where I can reproduce the syntax errors or the failed test report.
Thanks,
D.
There was a problem hiding this comment.
@TravisEz13 - I have tried to run the markdown syntax test locally. I have installed node-js and gulp as per the instructions in the script you linked, but it does not recognize 'test-mdsyntax' and I don't find it in the repo. I'm on Windows 10.
|
@TravisEz13 - I read the contributing information for pull requests and it says "clean up your commit history" I did that by rebasing - but now I'm wondering if the idea is to keep at least one new commit for each update to the PR? Let me know whether I should be squashing into my originally submitted commit or something else. |
|
@DarwinJS We must preserve the history, we should avoid rebaseing for already published commits. For new commits we should do one commit per one logical work piece: puttin 10000 g 10 commits with removing extra lines is bad. |
|
@TravisEz13 - I added to spelling exceptions to the repo root's .spelling - but they seem to be ignored when checking my .md in the tools subfolder. Should I create a .spelling in the tools subfolder - or is there something that has to be adjusted about the calling to the spelling checker? |
|
@DarwinJS Maybe you can do spell checking on WSL. |
|
@iSazonov - I neglected to mention I ran spell checking locally with the exact same code as used by CI and generated two spelling exceptions which it automatically added to .spelling in the repo root. However, when travis runs the build it does not seem to pickup on that .spelling file when it is processing the tools folder - so just trying to figure our the correct next step. |
|
@joeyaiello - this PR is being blocked by the "spelling errors" on 'sed' and '-includeide' in the md help document I put together. I have these exceptions in the main .spelling exception file and copied the .spelling file to the tools subfolder - still won't make it through. What else might I do to pass the checks? The command "powershell -File tools/travis.ps1" exited with 0. |
|
@DarwinJS You added "includeide" - try "-includeide". |
|
@iSazonov - I have tried you solutions in both .spelling files. There already was a newline after sed and the original additions to the file was done by interactively running mdspell. |
|
We can wrap "-includeide" with apostrophes `` in line 6 (and remove from |
|
mdspell runs clean on that file on my local system (windows 10) with the exact same .spelling file:
Is the version running in CI the latest? I am out of ideas :( |
|
@DarwinJS It seems a mdspell issue. Did you try to use apostrophes? |
|
@TravisEz13 and @iSazonov - Phew - Finally! |
|
Also, what is the plan on download.sh? |
|
I no longer understand what is holding up this PR - can someone please enlighten me? Thanks! |
|
Please resolve file conflicting. |
updating for pull request comments OSX Install script updated to use repositories Using TR, use curl rather than wget since we ensured it is on the system Update comments in install-powershell.sh install-powershell.sh
rename md to have only one period fix md spelling mistakes Fixes to markdown to follow strict syntax create readme
|
I forked this here: https://github.com/travisez13/powershell/tree/feature/install-powershell.sh and cleaned up the history, and re-based it against the current code. |
|
@TravisEz13 - I am interested in being listed as a contributor in this repo - will that still be the case with the actions you've taken? |
|
@DarwinJS , Yup, Only one commit I had to merge by hand got rewritten completely. You can see on 2 of the 3 remaining commits, you are listed as the main contributor: |
|
Thanks for your explanation and for your help @TravisEz13 ! |
|
@DarwinJS, I'll see if GitHub will let me update your PR (force push to your branch) once the tests pass. If that doesn't work, you are also welcome to pull my branch, then push it to your fork and submit the PR yourself, but it matters very little which of us submits the PR. |
|
@DarwinJS I reviewed the PR again. I enabled some tests on the markdown you added and fixed the issues it found and changed the default installer for PowerShell in our CI system to the one you wrote. I found the script didn't have execute permissions, I added them, and now I'm waiting for the tests again. |
|
@DarwinJS I think I have macOS working. I removed the openSSL because we already have code in start-psbootstrap to do this. There is a syntax error on line 99 of the macOS script, but it doesn't cause an issue, so, I'm not going to touch it for now. Using brew take about 150-160 seconds to install on the mac while download.sh took about 40 seconds. But bootstrapping is quicker by about the same time, So, I think this is acceptable. |
|
@TravisEz13 - thanks for all the fixups - exactly what I was asking for in one of the previous conference calls! |
|
I'm going to merge this soon... Please don't touch the branch. I'm going to clean-up the history first. |
|
@TravisEz13 - maybe this should just call install-powershell.sh now? |
|
@DarwinJS What is this? |
|
If you are talking about our CI systems, I mention previously, that I changed |
|
@TravisEz13 - apologies got this confused with one for download.sh |
|
@DarwinJS No Problem, This is merged. Thanks for your help. |
I am an intermediate-noob at bash so I would love to have some testing of the new installer before doing a pull request.
The goals and overall idea is described in #3547
Features (and fixes of download.sh):
I have it working and tested under:
Known Issues (and Help Needed):
Here is the readme: https://github.com/DarwinJS/PowerShell/blob/feature/install-powershell.sh/tools/install-powershell.readme.md
Here are the commands to kick off the install (also covered in readme):
Only Install PowerShell Core:
bash <(wget -O - https://raw.githubusercontent.com/DarwinJS/PowerShell/feature/install-powershell.sh/tools/install-powershell.sh)Install PowerShell Core with IDE:
bash <(wget -O - https://raw.githubusercontent.com/DarwinJS/PowerShell/feature/install-powershell.sh/tools/install-powershell.sh) -includeideInstall PowerShell Core with IDE and do tests that require a human to interact with the install process:
bash <(wget -O - https://raw.githubusercontent.com/DarwinJS/PowerShell/feature/install-powershell.sh/tools/install-powershell.sh) -includeide -interactivetesting