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

Use [:space:] instead of \s #10508

Merged
merged 1 commit into from
Feb 27, 2020
Merged

Use [:space:] instead of \s #10508

merged 1 commit into from
Feb 27, 2020

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Feb 27, 2020

Although modern versions of GNU grep make \s a synonym for [:space:] this is not POSIX compliant. We should use [:space:]

Fix #10507

@zeripath zeripath added type/bug topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile labels Feb 27, 2020
@zeripath zeripath added this to the 1.12.0 milestone Feb 27, 2020
@silverwind
Copy link
Member

@filipnavara can you verify this works in your MSYS?

$ printf "%03d%03d%03d" $(go version | grep -Eo '[0-9]+\.?[0-9]+?\.?[0-9]?[[:space:]]' | tr '.' ' ')
001013008

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 27, 2020
@filipnavara
Copy link
Contributor

@silverwind Yes, works with both GNU grep 2.5.4 and 3.1 on my machine.

@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 27, 2020
@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 27, 2020
@guillep2k
Copy link
Member

make is escaping the backslashes for the dots. I'm trying to write a small test case.

@codecov-io
Copy link

Codecov Report

Merging #10508 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10508      +/-   ##
==========================================
+ Coverage   43.67%   43.69%   +0.01%     
==========================================
  Files         586      586              
  Lines       81418    81418              
==========================================
+ Hits        35560    35572      +12     
+ Misses      41446    41435      -11     
+ Partials     4412     4411       -1
Impacted Files Coverage Δ
modules/indexer/stats/queue.go 62.5% <0%> (-18.75%) ⬇️
modules/indexer/stats/db.go 40.62% <0%> (-9.38%) ⬇️
modules/log/file.go 75.52% <0%> (-2.1%) ⬇️
models/notification.go 64.4% <0%> (+0.84%) ⬆️
modules/log/event.go 65.64% <0%> (+1.02%) ⬆️
services/pull/pull.go 36.07% <0%> (+1.17%) ⬆️
modules/queue/unique_queue_disk_channel.go 55.76% <0%> (+1.92%) ⬆️
modules/git/command.go 89.56% <0%> (+2.6%) ⬆️
services/pull/check.go 53.04% <0%> (+3.04%) ⬆️

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 a924a90...c873b92. Read the comment docs.

@zeripath zeripath merged commit 858aebc into go-gitea:master Feb 27, 2020
@guillep2k
Copy link
Member

I'm running this Makefile:

shell := /bin/bash

.PHONY: go-check
.PHONY: go-check1
.PHONY: go-check2
.PHONY: go-check3

all: go-check go-check1 go-check2 go-check3

go-check:
        $(eval GO_VERSION := $(shell printf "%03d%03d%03d" $(shell go version | grep -Eo '[0-9]+\.?[0-9]+?\.?[0-9]?[[:space:]]' | tr '.' ' ');))
        @echo "normal: $(GO_VERSION)"

go-check1:
        $(eval GO_VERSION1 := $(shell printf "%03d%03d%03d" $(shell echo go version go1.13.8 linux/amd64 | grep -Eo '[0-9]+\.?[0-9]+?\.?[0-9]?[[:space:]]' | tr '.' ' ');))
        @echo "check1: $(GO_VERSION1)"

go-check2:
        $(eval GO_VERSION2 := $(shell printf "%03d%03d%03d" $(shell echo go version go1x13x8 linux/amd64 | grep -Eo '[0-9]+\.?[0-9]+?\.?[0-9]?[[:space:]]' | tr '.' ' ');))
        @echo "check2: $(GO_VERSION2)"

go-check3:
        $(eval GO_VERSION3 := $(shell printf "%03d%03d%03d" $(shell echo go version go1x13x8 linux/amd64 | grep -Eo '[0-9]+\\.?[0-9]+?\\.?[0-9]?[[:space:]]' | tr '.' ' ');))
        @echo "check3: $(GO_VERSION3)"

And I get:

$ make
normal: 001013008
check1: 001013008
check2: 008000000
check3: 000000000

Bona-fide CentOS /bin/bash GNU bash, version 4.2.46(2)-release (x86_64-redhat-linux-gnu)

@zeripath zeripath deleted the fix-10507 branch February 27, 2020 18:03
zeripath added a commit to zeripath/gitea that referenced this pull request Feb 27, 2020
@silverwind
Copy link
Member

silverwind commented Feb 27, 2020

@guillep2k: I see no issue there, \. is meant to match literal .. I think the only character that really needs escaping in a a Makefile is $ via $$, otherwise shell escaping rules should apply, at least within single-quoted strings.

@guillep2k
Copy link
Member

guillep2k commented Feb 27, 2020

Yeah, it's not a problem, really. Only that . will match any character in regexp. But the preceding [0-9] will consume the digits first. It's only that invalid version go1Q13Q8 will be parsed as 1.13.8 instead of being rejected as an error.

@silverwind
Copy link
Member

It's only that invalid version go1Q13Q8

It should parse as 8. Could remove the first two ? in the pattern assuming version will always be two numbers at least.

zeripath added a commit that referenced this pull request Feb 27, 2020
@guillep2k
Copy link
Member

It's only that invalid version go1Q13Q8 will be parsed as 1.13.8

It should parse as 8. Could remove the first two ? in the pattern assuming version will always be two numbers at least.

@silverwind you're correct! as per my own comment: #10508 (comment) 😅

@zeripath zeripath added the backport/done All backports for this PR have been created label Feb 28, 2020
@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
backport/done All backports for this PR have been created 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 type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Go 1.14 version check fails on Windows/amd64
7 participants