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

Add missing windows extensions #1390

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add missing windows extensions #1390

wants to merge 2 commits into from

Conversation

kzu
Copy link

@kzu kzu commented Oct 29, 2024

When running on Windows, wdm is required in order to properly do incremental updates and regeneration.

The tzinfo-data dependency is also reported missing (under Windows at least, with jekyll 3.9.3).

@daattali
Copy link
Owner

I had no idea Windows even supports jekyll development! I'm a windows user but always assume anything I have to do has to happen on WSL on a remote server...

After looking at the docs, I see they recommend using the line ~> 0.1.1", :install_if => Gem.win_platform? , is there a reason your is a bit different? And the timezone gem I'd rather also only add on Windows since it doesn't seem to be required by other OS

@kzu
Copy link
Author

kzu commented Oct 31, 2024

Hm, I don't know how to do conditional deps. Total n00b on this. I just run jekyll ;)

@daattali
Copy link
Owner

I prefer not to modify sensitive files if we don't fully understand the impact. Modifying html/css/js files is easy, but anything to do with ruby I try to be very cautious about. If you test this on Unix and verify that it works , let me know.

@kzu
Copy link
Author

kzu commented Nov 1, 2024

Fixed with recommended suggestions! Run install/exec on WSL2 Ubuntu and it worked great too.

@kzu kzu force-pushed the wdm branch 2 times, most recently from 64d16cf to 97e28c9 Compare November 4, 2024 16:43
@daattali
Copy link
Owner

daattali commented Nov 6, 2024

I'm not sure why there are so many files changed in this PR

@ReenigneArcher
Copy link
Contributor

I'm not sure why there are so many files changed in this PR

I think something was accidentally included in the last force push.

@kzu
Copy link
Author

kzu commented Nov 7, 2024

Yep, fixing it now, sorry!

When running on Windows, wdm is required in order to properly do
incremental updates and regeneration.

Fix dependency on timezone to match docs

See https://jekyllrb.com/docs/installation/windows/#time-zone-management
@daattali
Copy link
Owner

daattali commented Nov 9, 2024

The indentation seems to be off (there's extra spaces in a few lines).

I'm not sure if these lines would also need to be included in the gemspec file, or does it make sense to only include them in the Gemfile? @ReenigneArcher do you have insight on this?

@ReenigneArcher
Copy link
Contributor

I don't have a ton of experience with ruby other than a little with this project and one other homebrew spec, but I found this blog post on the topic. https://yehudakatz.com/2010/12/16/clarifying-the-roles-of-the-gemspec-and-gemfile/

That seems to suggest the dependencies should be defined in the gemspec file versus the Gemfile.

Additionally with the gemspec file they can be properly specified as dev dependencies. I guess the syntax will be slightly different though.

spec.add_runtime_dependency "wdm", "~> 0.1.1", install_if: Gem.win_platform?

# Windows and JRuby does not include zoneinfo files, so bundle the tzinfo-data gem and associated library.
spec.add_runtime_dependency "tzinfo", ">= 1", "< 3", platforms: [:mingw, :x64_mingw, :mswin, :jruby]
spec.add_runtime_dependency "tzinfo-data", platforms: [:mingw, :x64_mingw, :mswin, :jruby]

@daattali
Copy link
Owner

daattali commented Nov 9, 2024

I read that as well, which is what made me question where this should be added. I'm going to install windowsruby myself and try this out and experiment. But I'll also post on a jekyll forum to get advice from people who have much more experience. I'm going to refrain from accepting this PR until know for sure that it's correct.

@daattali
Copy link
Owner

daattali commented Nov 9, 2024

@kzu can you clarify when exactly you ran into problems with Windows?

Did you see the installation instructions described here? Can you confirm whether Method 2 works for you - both option 1 and option 2?

@daattali
Copy link
Owner

I've spent a lot of time researching this, and I think the issue you're having is only when you clone this repository and try to run a jekyll site using the direct files in this repo, is that correct? If you use the theme using using jekyll new or as a gem, there should be no issues on windows, is that correct?

@daattali
Copy link
Owner

When using jekyll new, a Gemfile is created automatically. This Gemfile already has these tzinfo lines in it. It looks like even going back as far as jekyll 3.9.0 this was the case. Therefore, I believe that only the local Gemfile needs to be modified, and it would only solve the case of someone trying to run jekyll by directly cloning this repo. (This is not a common usecase, but it is nice to support it).

@daattali
Copy link
Owner

Before I finalize this, I asked on Jekyll forum and on SO, and I want to give it a few days to see if someone thinks my reasoning is wrong.

@kzu
Copy link
Author

kzu commented Nov 12, 2024

Yes, I think this is mostly for the case of someone contributing to this repo or forking to make changes (as I did). :)

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.

3 participants