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

Remove Dialyzer-specific problem matcher #182

Merged
merged 3 commits into from Mar 29, 2023

Conversation

btkostner
Copy link
Contributor

@btkostner btkostner commented Mar 23, 2023

Turns out this problem matcher is a little too loose, which causes it to match things it shouldn't. Noticed in this CI run, it will match timestamps in JSON. This can be seen if you run the common docker/setup-buildx-action@v2 (among others):

  {
    "nodes": [
      {
        "name": "builder-ebda29be-8062-4a4a-9e64-efec91d5e3610",
        "endpoint": "unix:///var/run/docker.sock",
        "status": "running",
        "buildkitd-flags": "--allow-insecure-entitlement security.insecure --allow-insecure-entitlement network.host",
        "buildkit": "v0.11.5",
        "platforms": "linux/amd64,linux/amd64/v2,linux/amd64/v3,linux/386"
      }
    ],
    "name": "builder-ebda29be-8062-4a4a-9e64-efec91d5e361",
    "driver": "docker-container",
    "lastActivity": "2023-03-23T23:22:29.000Z"
  }

Which would result in the last two lines being detected and a warning annotation on the last line:

    "driver": "docker-container",
    "lastActivity": "2023-03-23T23:22:29.000Z"
  Warning: }

Instead, it's recommended to run mix dialyzer --format github which will use a special GitHub actions log format to create warning annotations. This has the advantage of not being triggered for random log text. Running locally should output something like:

done in 0m1.07s
::warning file=lib/kafee/producer.ex,line=1,title=pattern_match::The pattern can never match the type true.
done (warnings were emitted)
Halting VM with exit status 2

Default output for comparison:

done in 0m1.09s
lib/kafee/producer.ex:1:pattern_match
The pattern can never match the type.

Pattern:
false

Type:
true

________________________________________________________________________________
done (warnings were emitted)
Halting VM with exit status 2

More information from the upstream PR: jeremyjh/dialyxir#463

@paulo-ferraz-oliveira
Copy link
Collaborator

Thanks for this, @btkostner.

A few requests/comments, though:

  1. could you share the output of using that option, for documentation purposes, in this pull request?
  2. I don't think the action should recommend using any specific options (thought this pull request will serve as a reference) since it's not specific to dialyzer/dialyxir
  3. I agree we can remove this, but want to have input from other maintainers (I too have seen it's too loose, but never really acted on it)

👍

@btkostner
Copy link
Contributor Author

  1. Updated PR body with more documentation and example code docs about the problem and replacement
  2. Agreed
  3. Sounds good 👍

@paulo-ferraz-oliveira
Copy link
Collaborator

Thanks for this, @btkostner. I've added a few other reviewers, for when they have time. At the same time, CI is failing, but it seems unrelated to this PR.

@paulo-ferraz-oliveira
Copy link
Collaborator

I'm squash-merging this without further approvals, as the approach seems sane. I hope to bump the action's version soonish after that.

@paulo-ferraz-oliveira
Copy link
Collaborator

Thanks for your contribution, @btkostner.

@paulo-ferraz-oliveira paulo-ferraz-oliveira changed the title fix: remove problematic dialyzer problem matcher Remove dialyzer problem matcher Mar 29, 2023
@paulo-ferraz-oliveira paulo-ferraz-oliveira changed the title Remove dialyzer problem matcher Remove Dialyzer-specific problem matcher Mar 29, 2023
@paulo-ferraz-oliveira paulo-ferraz-oliveira merged commit a255120 into erlef:main Mar 29, 2023
41 of 44 checks passed
@paulo-ferraz-oliveira
Copy link
Collaborator

v1.15.3 released

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.

None yet

2 participants