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

It seems vet on windows is unnecessary #14302

Merged
merged 4 commits into from
Jan 19, 2021

Conversation

lunny
Copy link
Member

@lunny lunny commented Jan 11, 2021

Fix the CI randomly failed.

@lunny lunny added the topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile label Jan 11, 2021
@jolheiser
Copy link
Member

jolheiser commented Jan 11, 2021

I would argue that vet on Windows is still useful. Consider when someone creates new OS-specific code and may forget to add copyright.

Aside from gitea-vet specifically, it would probably be nice to run the Go vet against it as well.

I haven't had a chance to take a look at what failed, but perhaps we could instead fix that.
EDIT: It seems like we just need to point it to gitea-vet.exe on Windows.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 11, 2021
@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 Jan 11, 2021
.drone.yml Outdated Show resolved Hide resolved
@lunny
Copy link
Member Author

lunny commented Jan 11, 2021

@jolheiser But in fact, the drone run on the container go1.15, it's a Linux container. We can compile gitea-vet.exe, but cannot run it.

@jolheiser
Copy link
Member

@jolheiser But in fact, the drone run on the container go1.15, it's a Linux container. We can compile gitea-vet.exe, but cannot run it.

Ah, yep you are right. I still think we should probably vet the Windows code (and other tags, like PAM) to make sure that any changes to them are vetted properly.

In this scenario, I guess we really just need to be able to run the linux gitea-vet binary against the code with Windows tags.

@silverwind
Copy link
Member

silverwind commented Jan 11, 2021

There has to be some better solution than having to run all lint/vet multiple times based on TAGS or GOOS because that method does not scale with the number of build tags. Can't we just somehow tell these tools to check all files, regardless of tags? They're both static analysis, right?

@lunny
Copy link
Member Author

lunny commented Jan 12, 2021

@silverwind It depends on the tools. But as I know, golangci-lint doesn't support scan all the files.

@silverwind
Copy link
Member

Opened golangci/golangci-lint#1646. Maybe it'll give us some clarification or lead to a feature to lint all files. I don't really know how these linters work so it may as well just not be possible.

@lunny
Copy link
Member Author

lunny commented Jan 18, 2021

@jolheiser @silverwind @6543 I have changed the PR, could you review it again?

@codecov-io
Copy link

codecov-io commented Jan 18, 2021

Codecov Report

Merging #14302 (c6e349d) into master (b59ed41) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14302      +/-   ##
==========================================
- Coverage   41.86%   41.85%   -0.01%     
==========================================
  Files         744      744              
  Lines       79746    79746              
==========================================
- Hits        33383    33376       -7     
- Misses      40853    40860       +7     
  Partials     5510     5510              
Impacted Files Coverage Δ
modules/util/timer.go 42.85% <0.00%> (-42.86%) ⬇️
modules/git/tree_entry_nogogit.go 87.50% <0.00%> (-6.25%) ⬇️
services/gitdiff/gitdiff.go 68.99% <0.00%> (-1.94%) ⬇️
modules/log/file.go 73.60% <0.00%> (-1.61%) ⬇️
modules/git/batch_reader_nogogit.go 46.26% <0.00%> (+1.49%) ⬆️
modules/charset/charset.go 73.03% <0.00%> (+2.24%) ⬆️
modules/git/repo_language_stats_nogogit.go 63.82% <0.00%> (+6.38%) ⬆️
modules/indexer/stats/db.go 68.00% <0.00%> (+12.00%) ⬆️

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 b59ed41...c6e349d. Read the comment docs.

@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 Jan 18, 2021
@6543 6543 merged commit b4dc080 into go-gitea:master Jan 19, 2021
@lunny lunny deleted the lunny/remove_vet_windows branch January 19, 2021 04:02
a1012112796 added a commit to a1012112796/gitea that referenced this pull request Jan 19, 2021
* master:
  Add pager to the branches page (go-gitea#14202)
  Removed invalid form tag (go-gitea#14391)
  Update back-up restore example for 1.13 changes (go-gitea#14374)
  It seems vet on windows is unnecessary (go-gitea#14302)
@go-gitea go-gitea locked and limited conversation to collaborators Mar 11, 2021
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.

8 participants