search

emacs-eldev / eldev

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

Fix/GitHub eldev install win #71

Closed ikappaki closed 2 years ago

ikappaki commented 2 years ago

Hi,

can you please consider patchfor github-eldev.bat installations script that fixes #70.

It appears that the MS-Windows github installation script does not set the eldev installation path correctly, it adds additional space at the end when running the echo, and thus the end path has an additional space at the end (!), which is invisible to the naked eye .... Not sure how this ever worked.

I've also included a new CI workflow that tests the github installation scripts across the supported architectures. This is a separate file, so that it does not interfere with the actual eldev tests.

Thanks,

ikappaki commented 2 years ago

It appears that having a separate github-eldev install test workflow clatters the CI view too much. I will try to merge it in test.yml.

ikappaki commented 2 years ago

So, I've merged the new github-eldev install tests into test.yml, replacing the existing github-eldev installation test which only tested that eldev.bat was installed, but not if it can be executed.

The window test is currently fails on CI because the actual fix it tests is not in master yet, where the github-eldev.bat is sourced from.

(I will squash when you give the go ahead, I wanted to keep the narration above valid)

doublep commented 2 years ago

Not sure how this ever worked.

Possibly it never did, I'm not sure if anyone uses this script on Windows. Does e.g. CIDER?

I've merged the new github-eldev install tests into test.yml

Perfect, just wanted to ask.

The window test is currently fails on CI because the actual fix it tests is not in master yet, where the github-eldev.bat is sourced from. - uses: actions/github-script@v3

I believe this was specifically converted to JS because of Windows support, see commit 44c49ae Use javascript-action instead of platform dependent Shell commands. Are you sure it is not needed now? Can it be that it fails because of that?

@juergenhoetzel: do you have any comments on this PR? It heavily touches code that you wrote.

ikappaki commented 2 years ago

Not sure how this ever worked.

Possibly it never did, I'm not sure if anyone uses this script on Windows. Does e.g. CIDER?

Nope, they are on CircleCI.

The window test is currently fails on CI because the actual fix it tests is not in master yet, where the github-eldev.bat is sourced from.

  • uses: actions/github-script@v3

I believe this was specifically converted to JS because of Windows support, see commit 44c49ae Use javascript-action instead of platform dependent Shell commands. Are you sure it is not needed now? Can it be that it fails because of that?

Indeed, it is different, but the new test is more conclusive I think:

  1. Existing test: run webinstall script and check if elder scripts has been installed at the expected location.
  2. New test: run web install script and test that you can call the installed eldev script successfully.

The problem with 1 was that although it checks whether the elder scripts was installed at the specified location, it does not really test if the script can be called and executed.

2 is better in the sense that it is more complete, that it also tests that the user can call the script successfully after installation. Do we also care to check if the script is installed at the local/bin location? in which case I think we can also keep the javascript code (slightly modified) as an additional step.

Thanks

doublep commented 2 years ago

Indeed, it is different, but the new test is more conclusive I think:

  1. Existing test: run webinstall script and check if elder scripts has been installed at the expected location.
  2. New test: run web install script and test that you can call the installed eldev script successfully.

[...]

2 is better in the sense that it is more complete, that it also tests that the user can call the script successfully after installation.

Agreed, this is better. I didn't look before because I usually just leave Windows improvements to you and Jürgen, since I don't even have access to that OS. But now I have checked the changes closer.

Do we also care to check if the script is installed at the local/bin location?

Not really important. On the other hand, since we remove installed Eldev to make other installation tests possible, we need to know what to remove. By the way, doesn't rm itself work as a test? I.e. won't it fail itself (and thus the workflow with it) if there is nothing to remove? Also, would be nice to test that Eldev cannot be executed before having run github-eldev[.bat].

If you can get rid of JS here, would it also be possible for other installation method tests? I see there are three in test.yml.

ikappaki commented 2 years ago

Do we also care to check if the script is installed at the local/bin location?

Not really important. On the other hand, since we remove installed Eldev to make other installation tests possible, we need to know what to remove. By the way, doesn't rm itself work as a test? I.e. won't it fail itself (and thus the workflow with it) if there is nothing to remove? Also, would be nice to test that Eldev cannot be executed before having run github-eldev[.bat].

Yes, rm is a test by itself, it will fail if eldev* is not there. Also I realised that both eldev help and rm should be on the same line, since we need both to succeed. I have also updated the github eldev installation jobs as suggested, to test for the presence of existing eldevinstallation and error out if they find one.

If you can get rid of JS here, would it also be possible for other installation method tests? I see there are three in test.yml.

Most probably yes, but I won't have time to look at the other js job with this PR unfortunately.

doublep commented 2 years ago

Also I realised that both eldev help and rm should be on the same line, since we need both to succeed.

Don't GitHub workflows fail if any command in a script fails, not only the last one?

Most probably yes, but I won't have time to look at the other js job with this PR unfortunately.

No problem. Would not quite be related to this PR anyway.

Please squash the commits.

ikappaki commented 2 years ago

Also I realised that both eldev help and rm should be on the same line, since we need both to succeed.

Don't GitHub workflows fail if any command in a script fails, not only the last one?

I don't think this is the always case; github invokes the shell of choice on the script and just inspects its return value. It's up to the shell to check each individual command and fail if is design so.

Please squash the commits.

Done, thanks!

doublep commented 2 years ago

Thank you. Let's hope that the failure is really gone once merged.