Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update scripts to run under windows #2698

Closed
wants to merge 3 commits into from
Closed

Conversation

rejas
Copy link
Collaborator

@rejas rejas commented Oct 20, 2021

Some people want to run MM under windows, so this PR adds a new start script entry without the display stuff.

It also makes the tests runnable under windows thanks to the cross-env library.

@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2021

Codecov Report

Merging #2698 (aa5f089) into develop (2b87d6e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #2698   +/-   ##
========================================
  Coverage    61.83%   61.83%           
========================================
  Files            8        8           
  Lines          262      262           
========================================
  Hits           162      162           
  Misses         100      100           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b87d6e...aa5f089. Read the comment docs.

@rejas
Copy link
Collaborator Author

rejas commented Oct 20, 2021

The alert test still seems unstable after #2687 @khassel ... Any idea on why it is so picky?

@khassel
Copy link
Collaborator

khassel commented Oct 20, 2021

no idea at the moment, can't reproduce it in my environment, it happens only in the ci jobs ... Alert could also be related to the refactor of this module ...

@rejas
Copy link
Collaborator Author

rejas commented Oct 20, 2021

By the way, this might be of help for win users until the script from @sdetweil mentioned here #2571 gets moved forward

@khassel
Copy link
Collaborator

khassel commented Oct 20, 2021

will increase the testTimeout to see if this solves the problem ...

@khassel khassel mentioned this pull request Oct 20, 2021
@sdetweil
Copy link
Collaborator

sdetweil commented Nov 6, 2021

is install fixed? vendor and fonts do not get installed as the script is wrong for windows batch

@sdetweil
Copy link
Collaborator

sdetweil commented Nov 6, 2021

if we went back to a script for start, then there could be two scripts
start_it (sh, but no extension)
start_it.cmd

then launch would the
"start":"./start_it"

@rejas
Copy link
Collaborator Author

rejas commented Dec 20, 2021

Argh, you're right @sdetweil windows-npm-install doesnt traverse into the font/vendor dirctories.

Having scripts seems like the only viable solution. But I am not sure if @MichMich wants more files in the repo.

But it would help for those who would want to develop on windows. So, he might say yes or no ;-) ?

@khassel
Copy link
Collaborator

khassel commented Dec 20, 2021

If @MichMich `s decision is to merge this, we should also update the documentation.

AFAIS there is no word about windows in the docs, so it should may mentioned here and also here.

May there are other places but searching does not work at the moment.

Tests are failing here due to:

npm ERR! code EBADPLATFORM
npm ERR! notsup Unsupported platform for fsevents@2.3.2: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})
npm ERR! notsup Valid OS:    darwin
npm ERR! notsup Valid Arch:  any
npm ERR! notsup Actual OS:   linux
npm ERR! notsup Actual Arch: x64

@rejas
Copy link
Collaborator Author

rejas commented Dec 20, 2021

Oh, this isnt ready to be merged with the windows install being not proper. Will convert it into a draft until we know how to proceed.

@rejas rejas marked this pull request as draft December 20, 2021 19:03
@MichMich
Copy link
Collaborator

I'm a bit conflicted with this PR. A while ago we made the decision to remove any non Raspberry support because it is simply not maintainable. The problem of adding these features is that people will rely on them and expect from us we make sure it keeps working. I personally have no interest or intention to maintain any windows support.

Since this PR does mainly add some convenience helpers it might be a better option to just add info to the docs.

What do you guys think?

@khassel
Copy link
Collaborator

khassel commented Dec 20, 2021

I personally have no interest or intention to maintain any windows support.

+1

new dependency cross-env:

🚨 NOTICE: cross-env still works well, but is in maintenance mode. No new features will be added, only serious and common-case bugs will be fixed, and it will only be kept up-to-date with Node.js over time.

I would have no problem with adding this so far but adding additional bash/cmd scripts to get the installation working on multiple platforms would be contrary to #1860

@rejas
Copy link
Collaborator Author

rejas commented Dec 21, 2021

Since this PR does mainly add some convenience helpers it might be a better option to just add info to the docs.

Funny, I looked in the docs and found something regarding this already in there: https://docs.magicmirror.builders/getting-started/installation.html#common-installation-issues

Maybe this section needs to be clearer about why it is there (non.raspberry installation)?

@rejas
Copy link
Collaborator Author

rejas commented Dec 21, 2021

Created a PR in the MM Docs repo. Maybe that might help?

@sdetweil
Copy link
Collaborator

but the doc IMPLIES that you SUPPORT running on Windows, which you "don't", so I would remove the doc too ..

you have to make up your mind.

@rejas
Copy link
Collaborator Author

rejas commented Dec 21, 2021

I didnt thought about that since stuff about other (not officially supported) installation methods is in the docs too

@khassel
Copy link
Collaborator

khassel commented Dec 21, 2021

Maybe this section needs to be clearer about why it is there (non.raspberry installation)?

yes, I saw this section and obviously the windows workaround is described there but windows is not mentioned ...

@MichMich
Copy link
Collaborator

I think the best thing is to close this issue and to improve the docs. I've applied for Agolia DocSearch to make searching the docs better.

@rejas
Copy link
Collaborator Author

rejas commented Dec 21, 2021

Closing in favor of MagicMirrorOrg/MagicMirror-Documentation#92

@rejas rejas closed this Dec 21, 2021
@rejas rejas deleted the win branch December 22, 2021 13:18
@rejas rejas restored the win branch August 6, 2022 14:05
@rejas rejas deleted the win branch October 1, 2022 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants