tests: Test on all os platforms

jcs090218 commented 1 year ago

I reckon it's better to test on all platforms? Just a suggestion.

andorsk commented 1 year ago

Looks like the windows checks are failing? Any thoughts here @jcs090218. The only other comment is I need to review the action you've built in more detail, as it's a young project and not a lot of eyes have hit it yet AFAIK.

jcs090218 commented 1 year ago

The issue is due to the DST Root CA X3 certificate, see https://github.com/jcs090218/setup-emacs-windows/issues/156#issuecomment-1126671598. I have added the workaround.

jcs090218 commented 1 year ago

I will recommend you use something like Eask or Eldev since you can test your package locally on your machine and not only on GHA. ;)

andorsk commented 1 year ago

@jcs090218 I took a further look at https://github.com/jcs090218/setup-emacs-windows/blob/master/action.yml and I had a few questions:

I noticed typescript-action, where this repo was generated and setup-emacs-windows are quite different. Is there a good reason? Same with jcs-emacs-setup. I haven't build any custom github actions, so sorry if this is a novice question.
Your package seems like a better version of purcell/setup-emacs@master, but it concerns me that there's so little community eyes on it so far ( using stars as a proxy ). I don't have the time to vet this entire package. Anything you can do to help me clear this? I want to trust your work here, but I also don't want to introduce a new dep that may have some weird consequence, with such a minor reason such as adding windows support. If I can get some support with why this should clear, I don't fundamentally have any issues with the PR otherwise. An alternative would be to use the old purcell with the switches you've suggested, but to remove the windows and osx support. Thanks again for the PR, and looking forward to adding this in if there's a better way to tell if this is safe to add.

andorsk commented 1 year ago

Also @jcs090218 wanted to thank you for sponsoring https://github.com/emacs-eask/cli. Open source patreons are wonderful and really appreciated!

jcs090218 commented 1 year ago

No worries! I am happy to answer you questions! 👍

I noticed typescript-action, where this repo was generated and setup-emacs-windows are quite different. Is there a good reason? Same with jcs-emacs-setup. I haven't build any custom github actions, so sorry if this is a novice question.

typescript-action is the template repo, so it's meant to be use as the template. If you go to their repo page, you should see Use this template green button where it should be the Clone button in the normal repo. More information, see https://docs.github.com/en/repositories/creating-and-managing-repositories/creating-a-template-repository.

Your package seems like a better version of purcell/setup-emacs@master, but it concerns me that there's so little community eyes on it so far ( using stars as a proxy ). I don't have the time to vet this entire package. Anything you can do to help me clear this?

I am not 100% sure, but here are my best guesses.

Emacser don't tend to support Native Windows, mostly on Linux > macOS > Windows + WSL.
If we compare the Used by sections from each GitHub repo page,

`jcs090218/setup-emacs-windows`	`purcell/setup-emacs`

purcell/setup-emacs (2.7k) has 5 times more usage than jcs090218/setup-emacs-windows (488).

There is no need to change. purcell/setup-emacs already supports Linux and macOS, there is no reason to change it to jcs090218/setup-emacs and they don't even think to support it.
Since jcs090218/setup-emacs is only 8 months old, it's a bit too late to introduce to the Emacs world. (purcell/setup-emacs is 3+more years)

I want to trust your work here, but I also don't want to introduce a new dep that may have some weird consequence, with such a minor reason such as adding windows support. If I can get some support with why this should clear, I don't fundamentally have any issues with the PR otherwise. An alternative would be to use the old purcell with the switches you've suggested, but to remove the windows and osx support. Thanks again for the PR, and looking forward to adding this in if there's a better way to tell if this is safe to add.

Thank you for trusting my work! ❤️ Many large projects have used jcs090218/setup-emacs-windows for a year or two and have no issue with it. Here is a list of project that uses the action,

For more, see https://github.com/jcs090218/setup-emacs-windows/network/dependents?dependents_after=MTk5NzU4MjA3NzI.

My intention for jcs090219/setup-emacs and Eask is to make Emacs development easier, esp. on the cross-platform part! 😄

Hope these information help!

andorsk commented 1 year ago

@jcs090218 sorry for the delay, and thank you for answering. I'm going to merge this into master and thank you again for the contribution!

The fact that some of these large projects ( spacemacs and lsp ) are using your package is really encouraging! Sorry if I had so many questions on this, but I wanted to be sure there was support enough to justify merging this into the mode.

I've starred your repo personally to help add some visibility to it.

jcs090218 commented 1 year ago

Sorry if I had so many questions on this, but I wanted to be sure there was support enough to justify merging this into the mode.

No problem! That's reasonable, and thank you for this awesome package! :D

I've starred your repo personally to help add some visibility to it.

Oh, nice! Thank you very much! 😁 ❤️ 🚀 🎉

andorsk / d2-mode

tests: Test on all os platforms #32