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

Makefile changes for Windows and easier development #6103

Merged
merged 9 commits into from
Feb 20, 2019

Conversation

jolheiser
Copy link
Member

@jolheiser jolheiser commented Feb 17, 2019

This just adds your go/bin to PATH and changes generate-stylesheets to use npx which is bundled with npm version 5.2+
If npx isn't found, it asks the user to update their npm to version 5.2+

This makes it easier for Windows devs, since Windows didn't like node_modules/.bin/lessc etc complaining about the path separator.
As well, it covers if a user hasn't already added their go/bin to their PATH, which would then make things like lint and misspell-check not work.

@jolheiser
Copy link
Member Author

@silverwind Any suggestions or things you don't like/would change?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 17, 2019
@codecov-io
Copy link

codecov-io commented Feb 17, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@eea1155). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #6103   +/-   ##
=========================================
  Coverage          ?   38.86%           
=========================================
  Files             ?      352           
  Lines             ?    50045           
  Branches          ?        0           
=========================================
  Hits              ?    19450           
  Misses            ?    27780           
  Partials          ?     2815

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 eea1155...99ff335. Read the comment docs.

@silverwind
Copy link
Member

If you remove node_modules/.bin it will use the global binaries instead of the ones we depend on. I suggest instead do npx lessc and similar. It will require node.js >= 8, but seeing as 6 is going EOL in 2 months, it should be acceptable.

@jolheiser
Copy link
Member Author

jolheiser commented Feb 17, 2019

Ah, okay. I had first tried that, but npx didn't seem to play nice with me. Any tips?

To clarify, npx lessc didn't seem to work, it wanted npx less.
And I never did get postcss to work for me.

@silverwind
Copy link
Member

Never tried it on Windows, but I assume it should just work. Tried updating node and/or npm?

@jolheiser
Copy link
Member Author

Using the current LTS of Node. Did a full reinstall for testing.
I'll mess with it a little more tonight and see what I can come up with.

On a separate note, since the local node modules is first in the path wouldn't it use that?

@silverwind
Copy link
Member

Would probably work, not sure if $PATH can really work properly with relative paths. npx uses node_modules and falls back to downloading the module if not found.

Makefile Outdated Show resolved Hide resolved
jolheiser and others added 2 commits February 17, 2019 20:35
Uses npx now for generate-stylesheets
Uses `go env GOPATH` to calculate adding GOPATH/bin to PATH
@jolheiser
Copy link
Member Author

jolheiser commented Feb 18, 2019

@silverwind Alright, it's using npx now. Turns out my npm path was messed up for some reason and that messed with npx

@zeripath I also changed to using $(go env GOPATH) to add to the path

@techknowlogick Pinging you as well since you had brought it up in the Discord. 👍

@lafriks lafriks added the topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile label Feb 18, 2019
Makefile Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 19, 2019
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 20, 2019
@zeripath
Copy link
Contributor

Oh before we merge this @jolheiser could you check the hacking documentation to make sure it doesn't conflict with this?

I think it's fine and if anything we suggest doing stuff that this makes unnecessary but just to be sure...

@jolheiser
Copy link
Member Author

The hacking documentation recommends adding Go to your path, which is unnecessary with this change, however as you said that's not necessarily a bad thing. This would just double it up.

As for the npx changes, currently the hacking documentation says to run npm install but doesn't actually mention that the user will need node (which would also contain npm).
The changes would check for npx and alert the user to update to a compatible version of npm, though.

Should I add a comment about installing node/npm before working with the stylesheets?

@silverwind
Copy link
Member

Should I add a comment about installing node/npm before working with the stylesheets?

Yes, I'd recommend we require Node.js >= 8.0 with npm included (to my knowledge, all distributions of Node.js include npm).

@jolheiser
Copy link
Member Author

Done

@silverwind
Copy link
Member

Would you mind also adding the version here?

@jolheiser
Copy link
Member Author

Done

@jolheiser
Copy link
Member Author

@zeripath I think this is ready now.

@zeripath zeripath merged commit eaf9ded into go-gitea:master Feb 20, 2019
@techknowlogick techknowlogick added this to the 1.8.0 milestone Feb 20, 2019
@jolheiser jolheiser deleted the windows_dev branch February 20, 2019 20:05
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants