emacs-eldev / eldev

Elisp development tool
https://emacs-eldev.github.io/eldev/
GNU General Public License v3.0
229 stars 17 forks source link

Add PowerShell/Windows support #26

Closed juergenhoetzel closed 3 years ago

juergenhoetzel commented 4 years ago

It would be great if there was a Powershell version. So I can replace this bad Cask hack.

Compared to the bash script temporary files are used for the elisp code because Powershell's handling of whitepace characters is a nightmare.

The Powershell bootstrap script ran right out-of-the-box without elisp changes. I was unsure what the `bootstrap.el.part and "eldev.in" files are used for. I copied the code from the bash script.

doublep commented 4 years ago

It would be great if there was a Powershell version.

Absolutely. However, It would be nice if you could also change README.adoc accordingly, or at least give me some information where it is useful (I have no idea), so I could adjust README.adoc myself.

Compared to the bash script temporary files are used for the elisp code because Powershell's handling of whitepace characters is a nightmare.

Is there absolutely no way do without such hacks? There are many cases where Eldev writes to both stdout and stderr (e.g.: eldev -t exec "("), and I fear it wouldn't work nicely with redirection. Also, what about colored output? But if your script handles that fine, then it's OK.

I was unsure what the `bootstrap.el.part and "eldev.in" files are used for. I copied the code from the bash script.

Create file bin/eldev.ps1.in (see bin/eldev.in for an example; everything contained between @[ and ]@ is evaluated as Elisp expression and replaced with its result). After that run eldev build. This will create bin/eldev.ps1 by combining your .in template and contents of bootstrap.el.part.

This would be less error-prone (copying of bootstrap Elisp code is automatic) and easier to review (I would only need to look at the template).

juergenhoetzel commented 4 years ago

Thanks for your quick feedback

It would be great if there was a Powershell version.

Absolutely. However, It would be nice if you could also change README.adoc accordingly, or at least give me some information where it is useful (I have no idea), so I could adjust README.adoc myself.

It is useful for Windows systems (The eldev shell-script doesn't work on Windows).

I use it to CI-test (GitHub Actions) fsharp-mode on Windows.

Compared to the bash script temporary files are used for the elisp code because Powershell's handling of whitepace characters is a nightmare.

Is there absolutely no way do without such hacks? There are many cases where Eldev writes to both stdout and stderr (e.g.: eldev -t exec "("), and I fear it wouldn't work nicely with redirection. Also, what about colored output? But if your script handles that fine, then it's OK.

I don't change existing elisp-code or redirect stdout/stderr. I just use --load (via temporary files) instead of --execute to load the elisp code. The Powershell code also checks for a terminal and sets ELDEV_TTY appropriately. Coloring works nice on the Windows Terminal:

fsharp-testing

I was unsure what the `bootstrap.el.part and "eldev.in" files are used for. I copied the code from the bash script.

Create file bin/eldev.ps1.in (see bin/eldev.in for an example; everything contained between @[ and ]@ is evaluated as Elisp expression and replaced with its result). After that run eldev build. This will create bin/eldev.ps1 by combining your .in template and contents of bootstrap.el.part.

This would be less error-prone (copying of bootstrap Elisp code is automatic) and easier to review (I would only need to look at the template).

I see. I will amend the commit.

doublep commented 4 years ago

It is useful for Windows systems (The eldev shell-script doesn't work on Windows).

I see. Please give some instructions how to install Eldev on Windows then, assuming your script is already present. E.g. on Linux, macOS etc. this involves modifying $PATH in environment (unless one wants to retype full path every time).

I just use --load (via temporary files) instead of --execute to load the elisp code.

Ah, I see. You said that it was necessary because of whitespace handling. Is it possible to just do something in the .in template and thus avoid temporary files? You have all of Elisp at your disposal to massage contents of bootstrap.el.part so that it can be used in Powershell script.

juergenhoetzel commented 4 years ago

It is useful for Windows systems (The eldev shell-script doesn't work on Windows).

I see. Please give some instructions how to install Eldev on Windows then, assuming your script is already present. E.g. on Linux, macOS etc. this involves modifying $PATH in environment (unless one wants to retype full path every time).

Create a directory for the bootstrap script:

mkdir ~\bin

Download the bootstrap script:

Invoke-WebRequest "https://raw.github.com/doublep/eldev/master/bin/eldev.ps1"  -OutFile ~\bin\eldev.ps1

Add the directory to your PATH:

$env:PATH += ";$env:USERPROFILE\bin"

Note: if you get an error you might need to change the execution policy (as administrator):

Set-ExecutionPolicy RemoteSigned -scope CurrentUser

I just use --load (via temporary files) instead of --execute to load the elisp code.

Ah, I see. You said that it was necessary because of whitespace handling. Is it possible to just do something in the .in template and thus avoid temporary files? You have all of Elisp at your disposal to massage contents of bootstrap.el.part so that it can be used in Powershell script.

Using shell-quote-argument did not fix the issues. I have had similar bad experiences with Powershell and quoting in the past. IMHO it is more robust to stay with temporary files. They will be deleted by Powershell in any case (even if the script is aborted).

doublep commented 4 years ago

@[(eldev-file-contents "bin/bootstrap.el.part" 0 1)]@

I presume escaping for PowerShell is not needed when assigning a multistring to a variable? Just asking.

"--debug",

Please remove this line. Eldev shouldn't run in debug mode by default, just like on other systems.

Otherwise it looks fine.

Can you create script(s) for installing Eldev on Windows, similar to webinstall/eldev and maybe some others in that directory, e.g github-eldev? Is there something similar to curl that is commonly available from PowerShell? Generally, what would be the shortest instruction to add to README? What you described above looks like quite many steps for a single file, or is nothing simpler possible on Windows?

Also, if you can do it, is it possible to adjust .github/workflows/test.yml to also test on Windows? I guess just like for macOS it is not necessary to test on all Emacs versions, just on the latest stable.

doublep commented 4 years ago

Looks perfect. Please adapt the URLs as if this is already merged and squash everything into one commit to the tune of "Add PowerShell scripts".

Documentation currently mentions this command for installing [on Linux]:

$ curl -fsSL https://raw.github.com/doublep/eldev/master/webinstall/eldev | sh

Can I add something similar for Windows now? I can write the text myself, just give me the command line. Sorry if this is taking too long, I just have nowhere to test and experiment myself.

I guess this needs to be merged first before trying something with automated testing, since as of now webinstall scripts are not available at the expected URLs, right?

juergenhoetzel commented 4 years ago

I guess this needs to be merged first before trying something with automated testing, since as of now webinstall scripts are not available at the expected URLs, right?

Yes, please do not merge this commits yet because I use paths to my own repo:

Invoke-WebRequest https://raw.githubusercontent.com/juergenhoetzel/eldev/powershell/bin/eldev.ps1 -Outfile "$DIR\eldev.ps1"                                                           

to test webinstalls: https://github.com/juergenhoetzel/github-actions-test-fsharp/actions

I will amend when I finished my tests

juergenhoetzel commented 4 years ago

While making the customizations for the GitHub actions, I unfortunately ran into more Windows platform problems:

(call-process "PATH_TO_ELDEV/bin/eldev.ps1")

results in

(file-error "Searching for program" #("Permission denied" 0 17 (charset windows-1252)) "PATH_TO_ELDEV/bin/eldev.ps1")

The reason is that win32.c does not consider ".ps1" as executable extension:

https://github.com/emacs-mirror/emacs/blob/78ec68e18f07a90a9ad400683b973ff51baa80e1/src/w32.c#L3490-L3499

static int
is_exec (const char * name)
{
  char * p = strrchr (name, '.');
  return
    (p != NULL
     && (xstrcasecmp (p, ".exe") == 0 ||
     xstrcasecmp (p, ".com") == 0 ||
     xstrcasecmp (p, ".bat") == 0 ||
     xstrcasecmp (p, ".cmd") == 0));
}
doublep commented 4 years ago

Hm, that's not good. Child Eldev processes are used not only during testing, but also for local dependencies. Can you investigate if there is any way to run eldev.ps1 script from inside Emacs? It's fine to add a special case for Windows here. If nothing works, maybe we can reimplement it to skip the script when invoking child Eldev process and instead go directly to launching child Emacs.

doublep commented 4 years ago

Also, is it really a problem of Emacs on Windows rather than OS itself? E.g. can you invoke .ps1 scripts using low-level C functions?

doublep commented 3 years ago

Ping for my latest two comments.

Do you think that if Eldev runs its child process directly as Emacs (essentially inlining eldev script) instead of using eldev[.ps1], this would solve troubles on Windows?

juergenhoetzel commented 3 years ago

Do you think that if Eldev runs its child process directly as Emacs (essentially inlining eldev script) instead of using eldev[.ps1], this would solve troubles on Windows?

It's not possible to spawn powershell scripts as native commands. I verified it using this snippet:

#include <stdio.h>
#include <process.h>

int main(int argc, char** argv) {
  const char *args[2] = {argv[1], NULL};
  printf("Spawning: %s\n", argv[1]);
  int result = _spawnv (_P_WAIT, argv[1], args);
  if (result == -1) {
    perror("spawn:");
  }
  printf("Return code: %d\n", result);
}

Tests:

$ ./spawn.exe hello.bat
Spawning: hello.bat

C:\Users\pidsleywin>echo hello
hello
Return code: 0

$ ./spawn.exe hello.ps1
Spawning: hello.ps1
spawn:: Exec format error
Return code: -1

I solved this issue by using a .bat wrapper script.

Now I ran in another issue: How do I pass a literal double quote from PowerShell to a native command?.

This breaks elisp coded passed to the spawned emacs:

emacs --batch --eval '(message "Hello")'
Symbol's value as variable is void: Hello

Inlining might solve this issue but will also complicate the code further.

Another possible solution would be to use temporary files (load instead of eval) for the elisp code.

doublep commented 3 years ago

Internally Eldev uses call-process to launch child processes. Would call-process-shell-command remove the need for .bat wrapper?

About quote escaping: does this also affect sth. like eldev eval "(+ 1 2)"?

juergenhoetzel commented 3 years ago

nternally Eldev uses call-process to launch child processes. Would call-process-shell-command remove the need for .bat wrapper?

It opens "notpad" :fearful: (because it's the associated application I guess)

About quote escaping: does this also affect sth. like eldev eval "(+ 1 2)"?

Fine:

.\bin\eldev eval "(+ 1 2)"
3

I guess we need some kind of powershell-quote-argument get correctly quote double quotes:

.\bin\eldev eval '(message """"Hello"""")'
Hello
"Hello
"
doublep commented 3 years ago

I guess we need some kind of powershell-quote-argument get correctly quote double quotes

There is already eldev-quote-sh-string, I wouldn't mind eldev-quote-powershell-string.

In general, however, this should only be necessary for internal invocation of child processes (probably also for eldev-message-command-line). We shouldn't try to alter the way PowerShell commands are typed in, because, I'd assume, users are accustomed and expecting the "standard" way. If some commands in the documentation don't work on PowerShell because of different quoting requirements, that's fine.

juergenhoetzel commented 3 years ago

I have not given up on Windows yet :smile:

I will tackle the one implementation of eldev-quote-powershell-string in the next few days.

ikappaki commented 3 years ago

Hi @juergenhoetzel

I have submitted a draft PR #35 to support Eldev on MS-Windows natively using .bat files. I quickly glanced overt this PR at the time and I thought it was stalled from 30 Dec 2020 (last comment), but I can now see you have made a commit on 9th Feb, just a little more than a month ago, so I assume you are still working on it?

I am not a friend of PS since I find it difficult to work with while I am more comfortable working with .bat files because, as in this case, they can be more closely modeled after the original shell scripts they complement and are much easier to follow up. However, in this instance a lot of work has already been covered with the powershell scripts thus I could potententially drop support of .bat files in my PR and concentrate on the test suite and usuability fixes not covered in here. Please let me know how you suggest we move forward and if I could be of any help with the .bat wrappers (you mentioned quoting in one of the comments, I might be of some help in that front).

Thanks

juergenhoetzel commented 3 years ago

Hi @juergenhoetzel

I have submitted a draft PR #35 to support Eldev on MS-Windows natively using .bat files. I quickly glanced overt this PR at the time and I thought it was stalled from 30 Dec 2020 (last comment), but I can now see you have made a commit on 9th Feb, just a little more than a month ago, so I assume you are still working on it?

Although my version is basically usable, some Windows-specific problems has stalled the my work. These problems occur when you run the comprehensive test suite.

for example, the following unix-specific definition

(defvar eldev--test-shell-command (expand-file-name "bin/eldev" eldev-project-dir))

makes the self-install test fail:

[00:13.434]  Test eldev-install-self-1 condition:
[00:13.440]      (ert-test-failed
                  ((should
                    (file-executable-p installed-executable))
                   :form
                   (file-executable-p "c:/users/pidsleywin/eldev/test/.tmp/27.1/bin-dir/eldev")

I am not a friend of PS since I find it difficult to work with while I am more comfortable working with .bat files because, as in this case, they can be more closely modeled after the original shell scripts they complement and are much easier to follow up. However, in this instance a lot of work has already been covered with the powershell scripts thus I could potententially drop support of .bat files in my PR and concentrate on the test suite and usuability fixes not covered in here. Please let me know how you suggest we move forward and if I could be of any help with the .bat wrappers (you mentioned quoting in one of the comments, I might be of some help in that front).

Yes, I started with a powershell-only-version but realized this is a dead end because .ps1 files are not directly executable from within emacs (as documented in previous comments).

So I would also suggest to create a .bat only version (also the strange PS quoting rules: How do I pass a literal double quote from PowerShell to a native command?. have driven me crazy) and just use powershell für the web-installer.

ikappaki commented 3 years ago

So I would also suggest to create a .bat only version (also the strange PS quoting rules: How do I pass a literal double quote from PowerShell to a native command?. have driven me crazy) and just use powershell für the web-installer.

Sure thanks, I shall then go ahead with the .bat solution (currently passes the test suite, and in the process of incorporating feedback from PP) while we use this PR for the web installer.

(I can feel your pain with the quoting, I had also had to go the extra mile for quoting the boostrap program in the bat file, pure madness)

juergenhoetzel commented 3 years ago

My branch is currently failing on these tests:

3 unexpected results:
   FAILED  eldev-install-self-1
   FAILED  eldev-loading-modes-2
   FAILED  eldev-package-3

Your branch:

11 unexpected results:
   FAILED  eldev-help-1
   FAILED  eldev-help-2
   FAILED  eldev-help-command-1
   FAILED  eldev-help-command-2
   FAILED  eldev-help-missing-dependency-1
   FAILED  eldev-just-run-1
   FAILED  eldev-just-run-missing-dependency-1
   FAILED  eldev-loading-modes-2
   FAILED  eldev-no-littering-1
   FAILED  eldev-no-littering-2
   FAILED  eldev-require-version-help-1

I will try to merge the branches. if everything goes well, only one error should remain:

eldev-loading-modes-2

it looks like final windows support is within reach :+1:

ikappaki commented 3 years ago

Hi @juergenhoetzel

please don't merge as yet, review with @doublep is still ongoing :)

I don't see any failures on windows with current #35 if this is what you meant, at least on my PC. (There is a single failure with Emacs 28.5 that is failing across all architectures, which I can reproduce locally but haven't had the time yet to look at yet: eldev-emacs-project-isolation-2).

Could you please let me know how you run the test suite?

This is what I get from a fresh command prompt

Microsoft Windows [Version 10.0...]
(c) 2020 Microsoft Corporation. All rights reserved.

D:\src\eldev>set ELDEV_LOCAL=d:\src\eldev\

D:\src\eldev>bin\eldev emacs --version
GNU Emacs 27.1
Copyright (C) 2020 Free Software Foundation, Inc.
GNU Emacs comes with ABSOLUTELY NO WARRANTY.
You may redistribute copies of GNU Emacs
under the terms of the GNU General Public License.
For more information about these matters, see the file named COPYING.

D:\src\eldev>bin\eldev test
Eldev tests are fairly slow, since they spawn lots of child processes
Running 326 tests (2021-03-18 12:10:43+0000, selector `t')
...

Ran 326 tests, 323 results as expected, 0 unexpected, 3 skipped (2021-03-18 12:21:26+0000, 643.942485 sec)

3 skipped results:
  SKIPPED  eldev-init-hg-1
  SKIPPED  eldev-init-svn-1
  SKIPPED  eldev-package-3

Thanks! :)

juergenhoetzel commented 3 years ago

Hi @juergenhoetzel

please don't merge as yet, review with @doublep is still ongoing :)

Sorry, perhaps I have expressed myself in a misleading way: I just wanted to integrate and rebase your changes in my pull request.

I don't see any failures on windows with current #35 if this is what you meant, at least on my PC. (There is a single failure with Emacs 28.5 that is failing across all architectures, which I can reproduce locally but haven't had the time yet to look at yet: eldev-emacs-project-isolation-2).

Could you please let me know how you run the test suite?

I'm using (Powershell prompt):

PS C:\Users\pidsleywin\eldev> git branch
* feature/windows-support
master
powershell
PS C:\Users\pidsleywin\eldev> $env:PATH +=  ";$(get-location)\bin"
PS C:\Users\pidsleywin\eldev> $env:ELDEV_LOCAL="$(get-location)"
PS C:\Users\pidsleywin\eldev> eldev test

BTW: I also started work on enabling Windows tests into Github Actions:

https://github.com/doublep/eldev/pull/26/files#diff-faff1af3d8ff408964a57b2e475f69a6b7c7b71c9978cccc8f471798caac2c88

ikappaki commented 3 years ago

Hi @juergenhoetzel

BTW: I also started work on enabling Windows tests into Github Actions:

https://github.com/doublep/eldev/pull/26/files#diff-faff1af3d8ff408964a57b2e475f69a6b7c7b71c9978cccc8f471798caac2c88

PP has requested we enable Windows test into Github Actions before merging, in what stage are you currently in with this please ?

Thanks!

juergenhoetzel commented 3 years ago

PP has requested we enable Windows test into Github Actions before merging, in what stage are you currently in with this please ?

Thanks!

I would suggest you cherry-pick my https://github.com/juergenhoetzel/eldev/commit/7c80c45aaee780c7119bc9e30d9614e431194a4f into your branch

https://github.com/juergenhoetzel/eldev/runs/2207461814?check_suite_focus=true

Testing with your branch has once again revealed some peculiarities of powershell (Github Actions uses powershell for the commands):

CMD:

C:\Users\pidsleywin\eldev>eldev eval "eldev-shell-command"
#("eldev" 0 5 (charset cp858))
C:\Users\pidsleywin\eldev>eldev eval "(eldev-shell-command)"
#("eldev" 0 5 (charset cp858))

Powershell:

PS C:\Users\pidsleywin\eldev> eldev eval "eldev-shell-command"
#("\"C:\\Users\\pidsleywin\\eldev\\bin\\eldev.bat\"" 0 41 (charset cp858))
PS C:\Users\pidsleywin\eldev> eldev eval "(eldev-shell-command)"
"c:/users/pidsleywin/eldev/\"C:/Users/pidsleywin/eldev/bin/eldev.bat\""

This seems to be the root-cause:

PS C:\Users\pidsleywin\eldev> eldev eval '(message """File name %s is absolute: %s""" eldev-shell-command (if (file-name-absolute-p eldev-shell-command) """YES""" """NO"""))'
File name "C:\Users\pidsleywin\eldev\bin\eldev.bat" is absolute: NO
#("File name \"C:\\Users\\pidsleywin\\eldev\\bin\\eldev.bat\" is absolute: NO
" 10 51 (charset cp858))
juergenhoetzel commented 3 years ago

PS C:\Users\pidsleywin\eldev> eldev eval "eldev-shell-command"

("\"C:\Users\pidsleywin\eldev\bin\eldev.bat\"" 0 41 (charset cp858))

calling "eldev.bat" from Powershell adds literal double quotes to %0

juergenhoetzel commented 3 years ago

Hi @ikappaki

This commit https://github.com/juergenhoetzel/eldev/commit/712d66970c8a349b58b2f9651e144c977e0d56ac fixes the powershell quoting issue.:

https://github.com/juergenhoetzel/eldev/runs/2208147733?check_suite_focus=true

Just 7 tests produced an unexpected result

ikappaki commented 3 years ago

HI @juergenhoetzel

I would suggest you cherry-pick my juergenhoetzel@7c80c45 into your branch

https://github.com/juergenhoetzel/eldev/runs/2207461814?check_suite_focus=true

Very good work!

I had a play with this commit, it only requires a two lines change to switch using the Command Line shell (=> shell: cmd) on Windows and then all test pass!

    - name: Test the project
      if: "startsWith (matrix.os, 'windows')"
      shell: cmd
      run: |
        eldev -p -dtTC test --omit-backtraces --expect 200
        eldev -p -dtTC test --test-type integration --omit-backtraces --expect 5
      env:
        ELDEV_LOCAL: "."

The only think that fails now is Test Installation 2 job with the failing error:


Run ! test -x ~/.eldev/bin/eldev
ParserError: D:\a\_temp\fbb1fa07-9471-4696-a5d4-050704a8a622.ps1:2
Line |
   2 |  ! test -x ~/.eldev/bin/eldev
     |   ~
     | Missing expression after unary operator '!'.

How would you like to take this forward? Since this is your work and you know most about it, would you like to fix the last failure and review with PP the whole action here, or would you like me to merge it as is, fix the last error and review with PP as part of the other PR?

Thanks!

juergenhoetzel commented 3 years ago

I had a play with this commit, it only requires a two lines change to switch using the Command Line shell (=> shell: cmd) on Windows and then all test pass!

Nice :+1: Downside: The if-conditional prevents to run the test on non-windows platforms at all.

It seems to be impossible to set the windows shell in the matrix: https://github.community/t/using-matrix-to-specify-shell-is-it-possible/17065

I was able to work arround this issue by just using a simple PS1 wrapper script eldev.ps1:

$bat =  (get-item $PSCommandPath) -replace ".ps1$", ".bat"
& cmd /c $bat $args

Using this Script eldev is now also full functional for powershell users.

How would you like to take this forward? Since this is your work and you know most about it, would you like to fix the last failure >and review with PP the whole action here, or would you like me to merge it as is, fix the last error and review with PP as part of > the other PR?

Good plan: I will try to fix the last (web)installer issue and merge/rebase your commits into my branch.

ikappaki commented 3 years ago

I had a play with this commit, it only requires a two lines change to switch using the Command Line shell (=> shell: cmd) on Windows and then all test pass!

Nice 👍 Downside: The if-conditional prevents to run the test on non-windows platforms at all.

It seems to be impossible to set the windows shell in the matrix: https://github.community/t/using-matrix-to-specify-shell-is-it-possible/17065

I was able to work arround this issue by just using a simple PS1 wrapper script eldev.ps1:

$bat =  (get-item $PSCommandPath) -replace ".ps1$", ".bat"
& cmd /c $bat $args

Excellent news!

(I was under the impression that we wanted to invoke the tests primarily using bin/eldev.bat from the Command Prompt, i.e. :shell cmd, and the above if conditional did just that -- it has been tested to work across the whole matrix. I think I now understand what you are saying that we want the tests to be primarily invoked from PowerShell. Invoking the tests twice once from Powershell and then from the command prompt will not work as per the aforementioned link -- not to mention that they will take double the time to complete, running the tests on windows is super slow :) )

Using this Script eldev is now also full functional for powershell users.

Wunderbar!

How would you like to take this forward? Since this is your work and you know most about it, would you like to fix the last failure >and review with PP the whole action here, or would you like me to merge it as is, fix the last error and review with PP as part of > the other PR?

Good plan: I will try to fix the last (web)installer issue and merge/rebase your commits into my branch.

Great, just to elaborate a little bit more (assuming there are no objections from anyone):

  1. PR #35 passes the review and is ready to merge,
  2. In the meantime you are up-to-date with the above, this PR passes the review and is ready to merge.
  3. PP merges #35 to master,
  4. You rebase from master.
  5. PP merges this PR.

(I hope I'm not complicating things unnecessarily!).

Thanks!

juergenhoetzel commented 3 years ago

relint integration test are failing on Windows:

[01:07.079]  Test eldev-relint-project-b-1 condition:
[01:07.079]      (ert-test-failed
                  ((should
                    (string-match-p "string-match-p" stderr))
                   :form
                   (string-match-p "string-match-p" "Dependency `relint' is not available; cannot use linter `re'
             ")
                   :value nil))

It seems to be a package/elpa issue.

ikappaki commented 3 years ago

Hi @juergenhoetzel

relint integration test are failing on Windows:

[01:07.079]  Test eldev-relint-project-b-1 condition:
[01:07.079]      (ert-test-failed
                  ((should
                    (string-match-p "string-match-p" stderr))
                   :form
                   (string-match-p "string-match-p" "Dependency `relint' is not available; cannot use linter `re'
             ")
                   :value nil))

It seems to be a package/elpa issue.

where can I download the latest version of this work please? I have finished with the .bat work, and I can have a look at the latest failures, if you think I could be of some help.

Thanks,

juergenhoetzel commented 3 years ago

where can I download the latest version of this work please? I have finished with the .bat work, and I can have a look at the latest failures, if you think I could be of some help.

Thanks,

I just rebased my commits on your squashed commit. You can check the (integration) tests in this PR.

But I can also reproduce the issue on my local system using your branch in a cmd prompt:

C:\Users\pidsleywin\eldev>eldev.bat  --color=never -p -dtT test --test-type integration
[00:00.226]  Started up on Mon Apr 5 18:48:19 2021
[00:00.231]  Running on GNU Emacs 27.1 (build 1, x86_64-w64-mingw32)
              of 2020-08-21
[00:00.240]  Project directory: `c:/users/pidsleywin/eldev/'
[00:00.246]  No file `c:/users/pidsleywin/.eldev/config', not applying user-specific configuration
[00:00.256]  Loading file `Eldev'...
[00:00.261]  No file `Eldev-local', not customizing build
[00:00.277]  Executing command `test'...
[00:00.283]  Executing `eldev-executing-command-hook'...
[00:00.289]  Executing `eldev-test-hook'...
[00:00.293]  Using Eldev tests of type `integration'
[00:00.298]  Eldev tests are fairly slow, since they spawn lots of child processes
[00:00.350]  Preparing to load the project in mode `packaged'
[00:00.355]  Full command line (in directory `c:/users/pidsleywin/eldev/'):
               eldev.bat package --output-dir c:/users/pidsleywin/eldev/.eldev/27.1/local/generated --print-filename
[00:01.294]  Output of the child Eldev process:
[00:01.301]  Nothing to do
             c:/users/pidsleywin/eldev/.eldev/27.1/local/generated/eldev-0.8.2snapshot.tar
             up-to-date

[00:01.320]  All project dependencies (including those for set `test') have been installed already or are local
[00:01.338]  Found test `.el' files: `test/integration/buttercup.el', `test/integration/checkdoc.el', `test/integration/elisp-lint.el', `test/integration/global-cache.el', `test/integration/misc.el', `test/integration/package-lint.el', `test/integration/relint.el', `test/integration/undercover.el' (8)
[00:01.369]  Loading test file `test/integration/buttercup.el'...
[00:01.514]  Loading test file `test/integration/checkdoc.el'...
[00:01.524]  Loading test file `test/integration/elisp-lint.el'...
[00:01.539]  Loading test file `test/integration/global-cache.el'...
[00:01.553]  Loading test file `test/integration/misc.el'...
[00:01.572]  Loading test file `test/integration/package-lint.el'...
[00:01.599]  Loading test file `test/integration/relint.el'...
[00:01.611]  Loading test file `test/integration/undercover.el'...
[00:01.624]  Reading previous ERT test results from file `.eldev/27.1/test-results.ert'...
[00:01.671]  Running 21 tests (2021-04-05 18:48:21+0200, selector `t')
[00:06.109]     passed   1/21  eldev-checkdoc-project-a-1 (4.431546 sec)
[00:07.394]     passed   2/21  eldev-checkdoc-project-b-1 (1.277167 sec)

[00:09.232]  Ran Eldev as `c:/users/pidsleywin/eldev/bin/eldev --setup '(eldev-use-local-dependency "../dependency-a")' lint el --required' in directory `c:/users/pidsleywin/eldev/test/missing-dependency-a/'
[00:09.240]  Stdout contents:

[00:09.242]  Stderr contents:
             Dependency `elisp-lint' is not available; cannot use linter `el'
ikappaki commented 3 years ago

I just rebased my commits on your squashed commit. You can check the (integration) tests in this PR.

But I can also reproduce the issue on my local system using your branch in a cmd prompt:

C:\Users\pidsleywin\eldev>eldev.bat  --color=never -p -dtT test --test-type integration

[00:09.232]  Ran Eldev as `c:/users/pidsleywin/eldev/bin/eldev --setup '(eldev-use-local-dependency "../dependency-a")' lint el --required' in directory `c:/users/pidsleywin/eldev/test/missing-dependency-a/'
[00:09.240]  Stdout contents:

[00:09.242]  Stderr contents:
             Dependency `elisp-lint' is not available; cannot use linter `el'

Sorry, I've completely missed the integration tests! I've fixed the issue as per fix in latest commit https://github.com/doublep/eldev/blob/f8c6bdf7c3cfc7abf8d88e88d38fe1d731357ef7/eldev.el#L1708

will squash once review is successful => update: squashed.

thanks!

juergenhoetzel commented 3 years ago

@doublep Currently this action does not test the PR code:

    # Method 1: if you have a catch-all directory for executables.  We
    # don't test bootstrapping, as that is supposed to have been
    # tested by normal ERT tests.
    - name: Test installation 1
      run: |
        mkdir ~/fake-bin
        cd ~/fake-bin
        curl -fsSL https://raw.github.com/doublep/eldev/master/bin/eldev > eldev && chmod a+x eldev
        test -x ~/fake-bin/eldev

It will download and test the code from the master branch. I propose to use github-context expressions.

IMO eldev should also be executed (not just check the filesystem file modes). I would propose to replace the shell code above with portable javascript code: https://github.com/actions/github-script

on the other hand, bootstrapping has already been tested in the ERT tests (as you mention in the comment): can this github action step be removed altogether perhaps? It is actually only a cURL download that is being tested.

doublep commented 3 years ago

I propose to use github-context expressions.

I would propose to replace the shell code above with portable javascript code

Can you create a separate PR for this? I currently don't have time to investigate things and write changes myself. Ideally, also add a comment that mentions that users of Eldev don't need to use those complications, it's only for testing Eldev itself (or is it not?).

juergenhoetzel commented 3 years ago

Finally I managed to combine the work of @ikappaki and mine:

As in the previous commit, I have to use a workaround to test the actual PR code (chicken or the egg problem): I added an optional parameter for the download URL in the (github-)eldev.bat scripts.

As a side note: To make these bootstrapping scripts actually useful on Windows a new MELPA release is required.

doublep commented 3 years ago

@ikappaki: Can you please comment? As I understand, this PR now includes both your and Jürgen's work and supercedes #35, is that right?

.github/workflows/bytecompile.el

Is there any need to break this out other than style preference?

name: Set git to use LF

Please add a short comment why it is needed, especially since it doesn't say "run this step only on Windows" (even if it makes no effective difference on other OSes).

name: Add to PATH

Likewise. I see that a variant of this step also runs on non-Windows. Why is it needed if it has worked without this step before?

bin/eldev.ps1

I see there are now two files (.bat and .ps1) for Windows. Looks like .ps1 is a simple wrapper over the .bat file. I trust you that this is needed to make it work in all cases, but please add a comment to .ps1 explaining it.

Looks like we are finally close to getting it all merged!

One thing I forgot about: what about some documentation? At least some paragraphs about installing on Windows, and preferably also about using in CI, if possible. It is not needed to create a PR if you don't want, you can just make some drafts here and I will include them into README.adoc. Or you can create a PR against branch future-doc where I keep documentations about news in the upcoming release (I will likely edit it when merging).

As a side note: To make these bootstrapping scripts actually useful on Windows a new MELPA release is required.

Sure, this is going to be the main new feature in 0.9.

juergenhoetzel commented 3 years ago

.github/workflows/bytecompile.el

Is there any need to break this out other than style preference?

Unix multiline string do not work on Windows. At this point, an if-conditional for the Powershell platform would have been required.

name: Set git to use LF

Please add a short comment why it is needed, especially since it doesn't say "run this step only on Windows" (even if it makes no effective difference on other OSes).

✓ Done in the amended commit.

name: Add to PATH

Likewise. I see that a variant of this step also runs on non-Windows. Why is it needed if it has worked without this step before?

good point: This is just a leftover from my early porting efforts. I removed it. powershell/cmd also accept forward slashes so we can use absolute paths in a platform agnostic way.

bin/eldev.ps1

I see there are now two files (.bat and .ps1) for Windows. Looks like .ps1 is a simple wrapper over the .bat file. I trust you that this is needed to make it work in all cases, but please add a comment to .ps1 explaining it.

✓ Done in the amended commit: This was one of the zillion windows oddities when using powershell:

Debugger entered--Lisp error: (file-missing "Searching for program" "No such file or directory" "d:/a/eldev/eldev/\"D:/a/eldev/eldev/bin/eldev.bat\"")
               call-process("d:/a/eldev/eldev/\"D:/a/eldev/eldev/bin/eldev.bat\"" nil t nil "package" "--output-dir" "d:/a/eldev/eldev/.eldev/27.1/local/generated" "--print-filename")

Looks like we are finally close to getting it all merged!

One thing I forgot about: what about some documentation? At least some paragraphs about installing on Windows, and preferably also about using in CI, if possible. It is not needed to create a PR if you don't want, you can just make some drafts here and I will include them into README.adoc. Or you can create a PR against branch future-doc where I keep documentations about news in the upcoming release (I will likely edit it when merging).

:+1: Sure: I propose to document the usage on Windows (Github CI) in a seperate PR.

ikappaki commented 3 years ago

@ikappaki: Can you please comment? As I understand, this PR now includes both your and Jürgen's work and supercedes #35, is that right?

Yes, this seems to be the case. My suggestion earlier in this thread was to merge #35 to master and then have this PR rebased from there so as not to lose the paper trail, but please proceed with what you think is more convenient. Thanks!

doublep commented 3 years ago

@juergenhoetzel: Sorry for not replying earlier, I somehow missed your comment and thought you haven't addressed those minor points yet. Everything looks fine now, only documentation is missing. Please create a PR as you proposed against future-doc branch. Note that I will likely edit it heavily when merging, I'm picky about documentation wording etc.