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: update versions & FPGA: fix naked return error from linter #1477

Merged
merged 3 commits into from
Jul 20, 2023
Merged

Makefile: update versions & FPGA: fix naked return error from linter #1477

merged 3 commits into from
Jul 20, 2023

Conversation

hj-johannes-lee
Copy link
Contributor

New version of golangci-lint shows errors about return with having no value.

FPGA related codes have such, so fixed.

@hj-johannes-lee hj-johannes-lee marked this pull request as draft July 18, 2023 20:53
@mythi
Copy link
Contributor

mythi commented Jul 19, 2023

New version of golangci-lint shows errors about return with having no value.

FPGA related codes have such, so fixed.

What change in golangi-lint triggers the error? Did they change the nakedret linter default settings?

Makefile Show resolved Hide resolved
@hj-johannes-lee
Copy link
Contributor Author

What change in golangi-lint triggers the error? Did they change the nakedret linter default settings?

I thought nakedret became default after v1.52.2, but it seems that it is still kept as disabled by default (https://golangci-lint.run/usage/linters/#disabled-by-default). But, we enabled in .golangci.yml. I have no idea actually why it caused the errors.

@mythi
Copy link
Contributor

mythi commented Jul 19, 2023

I have no idea actually why it caused the errors.

can you find out please?

@hj-johannes-lee
Copy link
Contributor Author

I have no idea actually why it caused the errors.

can you find out please?

I really do not understand...
I tried to see the release notes of golangci-lint and nakedret but could not find anything that looks related to this error we get.
What I found out is:
with the old version (v1.52.2) and linter-settings set with 'max-func-lines: 1' which should show everything that has naked return, it does not show the errors we get in the newer version (v.1.53.X).

So, my guess is just that there was some logic change of calculating how many lines a func has (and I think there was a problem in the logic in the past so that it did not show the errors though they should have).

@mythi
Copy link
Contributor

mythi commented Jul 19, 2023

I tried to see the release notes of golangci-lint and nakedret but could not find anything that looks related to this error we get.

this was fixed in v1.53.0: golangci/golangci-lint#1317

@mythi
Copy link
Contributor

mythi commented Jul 19, 2023

I tried to see the release notes of golangci-lint and nakedret but could not find anything that looks related to this error we get.

this was fixed in v1.53.0: golangci/golangci-lint#1317

Given this ^, it's not

In the golangci-lint version < v1.53.0, some naked return errors
are not caught. Fix the caught errors in the version >= v1.53.0.

but

golangci-lint version < v1.53.0 used nakedret linter that did not check
return values in conditionals. That got changed in v1.53.0 and some
of our code starts failing because of naked returns from conditionals.

Update the code to get nakedret linter passing.

golangci-lint version < v1.53.0 used nakedret linter that did not check
return values in conditionals. That got changed in v1.53.0 and some
of our code starts failing because of naked returns from conditionals.

Update the code to get nakedret linter passing.

Signed-off-by: Hyeongju Johannes Lee <[email protected]>
Signed-off-by: Hyeongju Johannes Lee <[email protected]>
@codecov-commenter
Copy link

Codecov Report

Merging #1477 (d6859ea) into main (0337834) will not change coverage.
The diff coverage is 0.00%.

❗ Current head d6859ea differs from pull request most recent head bf286c6. Consider uploading reports for the commit bf286c6 to get more accurate results

@@           Coverage Diff           @@
##             main    #1477   +/-   ##
=======================================
  Coverage   50.10%   50.10%           
=======================================
  Files          43       43           
  Lines        4878     4878           
=======================================
  Hits         2444     2444           
  Misses       2295     2295           
  Partials      139      139           
Impacted Files Coverage Δ
pkg/controllers/iaa/controller.go 6.89% <ø> (ø)
pkg/fpga/dfl_linux.go 0.00% <0.00%> (ø)
pkg/fpga/intel_fpga_linux.go 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@hj-johannes-lee hj-johannes-lee marked this pull request as ready for review July 20, 2023 13:40
@mythi mythi merged commit 89986b9 into intel:main Jul 20, 2023
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