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

Windows support #22

Open
malarahfelipe opened this issue Aug 16, 2023 · 27 comments
Open

Windows support #22

malarahfelipe opened this issue Aug 16, 2023 · 27 comments
Assignees
Labels
help wanted Extra attention is needed type:feature New feature or request

Comments

@malarahfelipe
Copy link

Hello, there's any plan for windows support on this?
Thanks

@malarahfelipe malarahfelipe added the type:feature New feature or request label Aug 16, 2023
@jannis-baum
Copy link
Owner

jannis-baum commented Aug 16, 2023

Hi! Since I personally don't use or have much experience with Windows I am not planning to do this myself, but I will happily accept a PR that adds Windows support!

That being said, I suspect it should be relatively straight forward to make the necessary adjustments and they shouldn't be too many. My guess is that what doesn't work with Windows right now is

everything else should already be pretty platform-independent, but like I said, I unfortunately can't test it myself. So if you're interested, go for implementing it! I'll help as much as I can if you have any questions :)

@jannis-baum jannis-baum added the help wanted Extra attention is needed label Oct 26, 2023
@pidgeon777
Copy link

I'm also interested...

@jannis-baum
Copy link
Owner

I'm also interested...

Hello! Happy to hear that you are interested in Vivify! :)

Like I said, I won't add windows support myself since I don't have any windows machine and it would be impossible for me to maintain it. However, if someone wants to implement it and is ready to maintain it as well, I would happily accept a PR and also provide as much help and support as I can :) So if this sounds like something for you, let me know!

@Tweekism
Copy link
Collaborator

@jannis-baum So I'm having an initial look into the Windows thing (good reason to look a little more deeply into the code base while working on that architecture.md job for you).

Seems possible, there is a bit of work invovled.

image

First hurdle is the way we set the environment variables for dev mode.

For now I've just stripped them out under a new script called win-dev, but we'll need Windows native equivalents.

image

The mime-type issue you mention above has moved but still exists.

image

There are node packages that will detect mime types, I assume they are cross platform compatible? Will need to look into that.

And there is a whole bunch of weirdness going on with paths, eg: C:\C:\Users\.... and backslashes instead of slashes, etc.

image

Here is your mime-type error, as expected, for completeness...

image

But you can navigate around, look Windowsy stuff!

image
image

Then perhaps the biggest issue is as you say the viv app. As a bash script, that would probably just be a rewrite of a separate Windows version would you say? Good news is there is not much going on there, so should be fairly easy?

@jannis-baum
Copy link
Owner

Hey @Tweekism awesome, thanks a lot! That's great, would you be up for being in charge of Windows development & maintenance?

First hurdle is the way we set the environment variables for dev mode.

For now I've just stripped them out under a new script called win-dev, but we'll need Windows native equivalents.

Interesting! Are you running this on "Windows Windows" or on WSL? I have very little idea what I'm talking about here but I have heard of WSL, maybe that's an option to just develop from there? Or do Windows users usually work directly from PowerShell or something Windows-y?

There are node packages that will detect mime types, I assume they are cross platform compatible? Will need to look into that.

This definitely sounds good. If there is a good Node package it's probably better than running a subprocess from Node for that anyways, even on POSIX.

And there is a whole bunch of weirdness going on with paths, eg: C:\C:\Users.... and backslashes instead of slashes, etc.

Yes, this was expected ^^ What did you do to make that first screenshot work though? It seems like you have a POSIX path there? And also the last screenshots where you navigate around, why does that work and what does the URL say?

Then perhaps the biggest issue is as you say the viv app. As a bash script, that would probably just be a rewrite of a separate Windows version would you say? Good news is there is not much going on there, so should be fairly easy?

Yes, full viv-rewrite for Windows sounds plausible! And yes, not a ton going on, but also not the most simplistic shell script because it has to do some fancy process-y stuff, I don't know if you've already had a look at the code. In case you need help understanding, I can help, but I have no idea how to do the equivalent on Windows ^^

@Tweekism
Copy link
Collaborator

Hey @Tweekism awesome, thanks a lot! That's great, would you be up for being in charge of Windows development & maintenance?

Potentially, but I promise nothing yet! Until I can fully scope out how much there is to do 😅

Interesting! Are you running this on "Windows Windows" or on WSL? I have very little idea what I'm talking about here but I have heard of WSL, maybe that's an option to just develop from there? Or do Windows users usually work directly from PowerShell or something Windows-y?

This is on Windows native, I had no idea people are using NeoVim on Windows, so I couldn't tell you what the "average Windows vim" user is doing. I'll @ the users in this post separately and ask them.

Yes, this was expected ^^ What did you do to make that first screenshot work though? It seems like you have a POSIX path there? And also the last screenshots where you navigate around, why does that work and what does the URL say?

If you type in manually a POSIX path it works, but all the links generated by the directory renderer are messed up.

Screenshot from 2024-07-30 16-37-30

Yes, full viv-rewrite for Windows sounds plausible! And yes, not a ton going on, but also not the most simplistic shell script because it has to do some fancy process-y stuff, I don't know if you've already had a look at the code. In case you need help understanding, I can help, but I have no idea how to do the equivalent on Windows ^^

I've have a cursory glance. I'll definitely take up your offer to explain it a bit when I get to that part.

@Tweekism
Copy link
Collaborator

@malarahfelipe @pidgeon777 Hey guys, are you guys able to describe your setup a little bit?

Using Vim/NeoVim I assume? on Windows natively? Or WSL2? or something else?

@Tweekism Tweekism self-assigned this Jul 30, 2024
@Tweekism
Copy link
Collaborator

@jannis-baum Ok i quickly hit the limit of my typescripting ability lol.

This works in my simple test project (just normal node.js using yarn init, yarn add mime-types etc.

image
image

But when i go to add that in Vivify | ../path.ts I get this

image

I guess I'm missing some typescript magic? Is it simple or do I need to go do a bunch of reading 😅

@jannis-baum
Copy link
Owner

Ah, yes, quick TypeScript lesson I guess haha:

On npmjs.com we get information about the typing of the package next to the title. There are three scenarios

  1. (best) it has a "TS"-logo so it's natively typed → do nothing and it works in TypeScript
  2. (still very acceptable) it has a "DT"-logo so it's not natively type but there are types → do yarn add --dev @types/<package-name> and it works
  3. (bad) it has no logo, there are no types → pain

In this case we are in scenario 2:

image

So you can run yarn add --dev @types/mime-types to install the types as a dev dependency and then TypeScript will have nothing to complain about anymore :)

@Tweekism
Copy link
Collaborator

Oh ok, well in that case there might be better packages out there for typescript, of the three I tried this one worked the easiest for me, ok let me go try some stuff and I'll be back :)

@jannis-baum
Copy link
Owner

Oh ok, well in that case there might be better packages out there for typescript, of the three I tried this one worked the easiest for me, ok let me go try some stuff and I'll be back :)

Having types in a separate package is totally fine, that happens a lot. Nothing to worry about. I would put your subjective assessment of the interface and overall popularity above the criteria of not having native types. If it wasn't typed at all it would be a different story but this one is fine if you like it! Also seems very popular.

@Tweekism
Copy link
Collaborator

it is one of the popular ones. mime-type is a fork of it for TS apparently. I ran the command you gave me and I get a different error, but I haven't looked into that yet. I don't want to bother you every 5 seconds lol.

@jannis-baum
Copy link
Owner

No worries, I'm happy to help :) Let me know what it is in case you can't figure out that new error

@Tweekism
Copy link
Collaborator

Its ok, I fixed it, still something a little weird going on, but i think its something to do with typescript protecting me from possible nulls/undefines?

Anyway I got it working.

AND it renders on Windows as well...

image

@Tweekism
Copy link
Collaborator

Ok so here is the list of minimum changes to get server working:-

  • Setup Windows specific environment variables
  • Replace the mime type call with something cross platform
  • Strip leading backslashes \ from file paths, like we do for forward slashes / (having backslashes after that is ok, browsers in Windows are setup to expect that).

So far everything I've done are just hacks for exploratory purposes and so comes with the following limitations:-

  • You can't use shortened home paths (~)
  • You can't access anything but C:\
  • I haven't looked at the viv client at all yet, so you have to manually invoke the browser
  • And this mime type library is a bit crap, it only looks at file extensions so it can't tell the difference between a text file with no extension (eg: LICENSE) and a binary file (like: vivify-server)

There is a package called File-Types that can get mime-types that might be better, it uses async calls so perhaps that means its actually looking at the file. @jannis-baum I'll make a test project and see if that library is better, but I may need a hand implementing that one into the main project 😅, but at least that can be done and tested on MacOS / Linux.

Most the work here will be around wrangling paths. Like how do we want to implement drive letters on Windows? It would be fairly likely that someone has files on an external D:\ drive for example. Will need some feedback on that.

Screencast from 2024-07-31 11-46-31

Go here to see the changes on my branch: Tweekism/vivify@main...windows-support

@jannis-baum
Copy link
Owner

Nice this looks good!

There is a package called File-Types that can get mime-types that might be better, it uses async calls so perhaps that means its actually looking at the file. @jannis-baum I'll make a test project and see if that library is better, but I may need a hand implementing that one into the main project 😅, but at least that can be done and tested on MacOS / Linux.

I can take over for this part and do it as a separate issue/PR :) I guess the package you are referring to is this one? https://www.npmjs.com/package/file-type

Most the work here will be around wrangling paths. Like how do we want to implement drive letters on Windows? It would be fairly likely that someone has files on an external D:\ drive for example. Will need some feedback on that.

Hm, there are multiple options here that come to mind. But before I have a couple of questions from looking at the code and your screencast:

  1. I am not quite sure why you even get \ in the URLs in your screencast because pathToURL should encode them to %5C. We explicitly decode forward slashes again here but not backslashes so why do they end up in the URL?
  2. What happens when you just try to access a URL with a drive letter? Maybe it just works already, either literally with /viewer/D:\my\file or URI-encoded with /viewer/D%3A%5Cmy%5Cfile?

The best option I can come up with right now would probably be to "virtualize" the drives as a root directory so that we can browse through them with Vivify (i.e. when you get down to C:\ you can press the up button again to select a drive). Not sure how much work this would be and if we could nicely wrap everything so we have minimal difference between the Windows and POSIX implementations

@Tweekism
Copy link
Collaborator

Tweekism commented Aug 1, 2024

I can take over for this part and do it as a separate issue/PR :) I guess the package you are referring to is this one? https://www.npmjs.com/package/file-type

Hold on that, I tested it and it doesn't work.

I tried a WHOLE bunch of different npm packages and they essentially fall into 2 categories:

  1. They rely on the file explicitly telling them what kind of file they are, either by the extension or in the file header, or
  2. They use the system file command under the hood as you have done.

Option 1 is a noticable downgrade from what we currently have and option 2 won't work on Windows which obviously is the whole point.

I have some thoughts on this I'll go into details later, this is just a quick reply for the moment.

Hm, there are multiple options here that come to mind. But before I have a couple of questions from looking at the code and your screencast:

  1. I am not quite sure why you even get \ in the URLs in your screencast because pathToURL should encode them to %5C. We explicitly decode forward slashes again here but not backslashes so why do they end up in the URL?

This is Firefox converting them back, on Edge (chormium) I get %5C

  1. What happens when you just try to access a URL with a drive letter? Maybe it just works already, either literally with /viewer/D:\my\file or URI-encoded with /viewer/D%3A%5Cmy%5Cfile?

I can't test at the moment but essentially you get something like 'File not found: C:\C:\path\to\some\file'

The best option I can come up with right now would probably be to "virtualize" the drives as a root directory so that we can browse through them with Vivify (i.e. when you get down to C:\ you can press the up button again to select a drive). Not sure how much work this would be and if we could nicely wrap everything so we have minimal difference between the Windows and POSIX implementations

That Is the usual case I see on windows. something like .../c/Users/jason/blah...

Like I say, I'll come back to this a bit later when I have some time, but I just didn't want to leave you thinking that file-type package was the way forward, because I don't think it is, at least not on its own and even then, it's no better than any of the other packages out there. It seems to me that we might end up with a hybrid solution, something like use file if its available. If not, fall back to what is essentially a file extension lookup table (aka: one of these mime packages)

@jannis-baum
Copy link
Owner

Option 1 is a noticable downgrade from what we currently have and option 2 won't work on Windows which obviously is the whole point.

Hm, but shouldn't looking at the file header be all we need? If you look at the following

https://github.com/jannis-baum/Vivify/blob/main/src/routes/viewer.ts#L48-L51

we switch between plain text vs. binary and only really need the mime type for binary files, which the packages that look at the file header should be able to figure out. If it's plain text we handle it later in the parser anyways. So I think this package we were talking about should do it, shouldn't it?

I'm pretty sure file is also no dark magic, probably just looks at the header, then at the extension, and otherwise says "plain text" ^^

This is Firefox converting them back, on Edge (chormium) I get %5C

Ah, interesting!

I can't test at the moment but essentially you get something like 'File not found: C:\C:\path\to\some\file'

That's weird, what's the path it has in the GET /viewer route? Does that already have the double drive?

@Tweekism
Copy link
Collaborator

Tweekism commented Aug 1, 2024

Hm, but shouldn't looking at the file header be all we need? If you look at the following

In practice it turns out no. Some files like png, jpeg, pdf etc. will explicitly say what they are in the first few bytes of the file.

For example, take this beautiful .png my friend made me for my steam avitar:

image

When you open it in a text editor you'll notice its very up front about being a .png like it's proud of it or something...

image

However this isn't always the the case for plain text, often its just a stream of ASCII (or utf-8) bytes. Ditto with binary files, they can just be what ever. Which is why the file project calls itself a file type "guesser"

The practical implication is that every one of the "category 1" npm libraries I tryed failed to return any mime type at all on our LICENSE file or the viv and vivify-server executables, as they all have no file extenstions. And some would also fail on my test png if i removed the extension, while others would recognise it by the header

image

Note that some plain text files do Identify themselves, like this one:

image

That is some kind of unicode specifier, I don't remember the exact details, I'd have to look it up (this one is UTF-8 obviously, you'll see UTF-16 a bit in MS land or I suppose UCS2 kinda sorter 'cus history etc. but its obvious to spot with all the zeros '00')

https://github.com/jannis-baum/Vivify/blob/main/src/routes/viewer.ts#L48-L51

we switch between plain text vs. binary and only really need the mime type for binary files, which the packages that look at the file header should be able to figure out. If it's plain text we handle it later in the parser anyways. So I think this package we were talking about should do it, shouldn't it?

They should, but they don't. And to be fair they'd have to be slightly more sophisticated and do some heuristic analysis on the file as a whole and not just look at the header to be able to tell plain text from binary.

I'm pretty sure file is also no dark magic, probably just looks at the header, then at the extension, and otherwise says "plain text" ^^

Turns out it is a little dark magic (who knew right?) and it is not always correct, in fact on Linux it identifies our .md files an text/plain instead of text/markdown, but that could just be because it's old. Actually I noticed that xgd-mime does a much better job.

This is Firefox converting them back, on Edge (chormium) I get %5C

Ah, interesting!

Indeed, the sly fox...

I can't test at the moment but essentially you get something like 'File not found: C:\C:\path\to\some\file'

That's weird, what's the path it has in the GET /viewer route? Does that already have the double drive?

Ummmm.... I'll test soon and let you know.

@Tweekism
Copy link
Collaborator

Tweekism commented Aug 1, 2024

And having said all that there are some pretty easy work arounds, so I'm not all that worried about this bit anymore, but I'm expecting visitors and fam is starting dinner without me so I'll be back :)

@jannis-baum
Copy link
Owner

In practice it turns out no. Some files like png, jpeg, pdf etc. will explicitly say what they are in the first few bytes of the file.

But, in practice, aren't these binary files that announce their file type in the header exactly the only ones that we need to know the mime type of? The main reason we need to know mime types at all is so that we can send them as files on the GET endpoint so that e.g. local images render in Markdown files. For generic binaries, e.g. some strange executable, we don't really care what it is. If we don't "understand" it, we can also just show an error page there that says "unknown file type" or something, it won't hurt Vivify's functionality.

And to be fair they'd have to be slightly more sophisticated and do some heuristic analysis on the file as a whole and not just look at the header to be able to tell plain text from binary.

The part that does seem significant is being able to recognize that a file is not binary, this is something we definitely have to be able to do so that we can run our rendering endpoint on it.

And, another topic in case you haven't considered that: There are some plain-text files that have to be sent as files, e.g. SVG images. So this is also something that needs to have its mime type recognized.

And having said all that there are some pretty easy work arounds, so I'm not all that worried about this bit anymore, but I'm expecting visitors and fam is starting dinner without me so I'll be back :)

I'm curious!


By the way I just saw there is also file for Windows. I think it would be completely fair to have a dependency on that for Windows. I'd honestly prefer that over doing some hacky stuff and/or implementing too much of this ourselves in Vivify (the less stuff we have here, the less prone we are to errors and the easier it is to maintain, etc.)

@Tweekism
Copy link
Collaborator

Tweekism commented Aug 3, 2024

By the way I just saw there is also file for Windows.

Ok, lets do this! I tested it, it works and then the implementation is identical everywhere.

PS, I have rewritten this post about 6 times. Thousands of words, none of it very useful. Maybe I should start keeping a diary instead...

@jannis-baum
Copy link
Owner

Ok, lets do this! I tested it, it works and then the implementation is identical everywhere.

Great! So then even the child process call it the exact same? That's very cool.

PS, I have rewritten this post about 6 times. Thousands of words, none of it very useful. Maybe I should start keeping a diary instead...

Haha no worries, I do this as well and I also like reading along and seeing how it's going for others ^^ Just not sure about the people who have requested this, if they're still subscribed they always get emails from our conversation. If you want you could already open a draft PR and we keep talking there

@pidgeon777
Copy link

I would like to extend my gratitude to everyone involved in the efforts to make Vivify compatible with Windows. If needed, I am available to offer my assistance as a tester.

@Tweekism
Copy link
Collaborator

Tweekism commented Aug 6, 2024

Hey @pidgeon777

I'll take you up on that offer once the time comes :)

I've not worked on this in the last couple days but will be back on to it starting tomorrow.

@Tweekism
Copy link
Collaborator

Tweekism commented Aug 7, 2024

@jannis-baum Since this could be a fairly long running draft PR and with external testers (@pidgeon777 and hopefully others), do you want me to push the development branch here, or to my fork?

@jannis-baum
Copy link
Owner

@jannis-baum Since this could be a fairly long running draft PR and with external testers (@pidgeon777 and hopefully others), do you want me to push the development branch here, or to my fork?

Here is good! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed type:feature New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants