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

Is GitHub Actions example correct? #497

Open
luigi-discoup opened this issue May 15, 2023 · 11 comments
Open

Is GitHub Actions example correct? #497

luigi-discoup opened this issue May 15, 2023 · 11 comments

Comments

@luigi-discoup
Copy link

luigi-discoup commented May 15, 2023

Precheck

  • Take a look at the open issues and be sure that your issue is not already covered.
  • Be sure your versions of Dialyxir and Erlex are up to date.

Environment

  • Elixir & Erlang/OTP versions (elixir --version): Elixir 1.14.4 (compiled with Erlang/OTP 25)

  • Which version of Dialyxir are you using? (cat mix.lock | grep dialyxir): "dialyxir": {:hex, :dialyxir, "1.3.0", "fd1672f0922b7648ff9ce7b1b26fcf0ef56dda964a459892ad15f6b4410b5284", [:mix], [{:erlex, ">= 0.2.6", [hex: :erlex, repo: "hexpm", optional: false]}], "hexpm", "00b2a4bcd6aa8db9dcb0b38c1225b7277dca9bc370b6438715667071a304696f"},

Question

From the Continuous Integration documentation page, I see that I have to put plt_file: {:no_warn, "priv/plts/dialyzer.plt"} in the dialyzer project property and configure the GitHub Actions cache as described here, but I'm not sure about this configuration.

Using the GitHub Actions cache configuration provided, the PLT cache will be stored only once, and every subsequent changes in source code or dependencies will be analyzed again and never stored in cache so the project specific PLT wil be generated every time. Am I right?

As far as I understand, reading the PLT section, a better configuration would be to put plt_core_path: "priv/plts" in dialyzer configuration instead of plt_file, leaving the local PLT file in its default location _build/env/dialyze_erlang-[OTP Version]_elixir-[Elixir Version]_deps-dev.plt and caching the _build folder separatly.

@jeremyjh
Copy link
Owner

jeremyjh commented May 15, 2023

The cache key name includes the OTP version and the Elixir version, so the cache will be updated when those versions change. Still, I agree this is probably not quite correct, the key name should probably also include a hash of mix.lock. Otherwise some extra work will be done on each subsequent build after dependencies are changed. It looks like all the CI examples have this same issue. In practice it may not slow down the jobs too much but I agree it does not seem ideal to offer an example like this.

I'm curious for @Nezteb's take just in case I'm missing something here as he added the Gitlab example relatively recently and it uses mix.lock in the _build cache but not the PLT cache.

@luigi-discoup
Copy link
Author

luigi-discoup commented May 15, 2023

@jeremyjh yes the chache name includes OTP and Elixir version and it works well for core PLTs, but project specific PLT need extra work at every run.

I've just configured my repo in this way and it seems to work:

mix.exs

defmodule MyApp.MixProject do
  use Mix.Project

  def project do
    [
      ...
      dialyzer: [plt_core_path: "priv/plts"],
      ...
    ]
  end

  ...
end

.github/workflows/build.yaml

name: Elixir build

...

jobs:
  checks:
    name: Analyze and test
    steps:
      - name: Git clone the repository
        uses: actions/checkout@v3
      - name: Set up Elixir
        id: beam
        uses: erlef/setup-beam@v1
        with:
          elixir-version: "1.14.4"
          otp-version: "25"
      - name: Build cache
        uses: actions/cache@v3
        with:
          path: |
            deps
            _build
          key: ${{ runner.os }}-mix-${{ hashFiles('**/mix.lock') }}
          restore-keys: ${{ runner.os }}-mix-
      - name: Install dependencies
        run: mix deps.get
      - name: Restore PLT cache
        id: plt_cache
        uses: actions/cache/restore@v3
        with:
          key: ${{ runner.os }}-${{ steps.beam.outputs.elixir-version }}-${{ steps.beam.outputs.otp-version }}-plt
          restore-keys: ${{ runner.os }}-${{ steps.beam.outputs.elixir-version }}-${{ steps.beam.outputs.otp-version }}-plt
          path: priv/plts
      - name: Create PLTs
        if: steps.plt_cache.outputs.cache-hit != 'true'
        run: mix dialyzer --plt
      - name: Save PLT cache
        id: plt_cache_save
        uses: actions/cache/save@v3
        if: steps.plt_cache.outputs.cache-hit != 'true'
        with:
          key: ${{ runner.os }}-${{ steps.beam.outputs.elixir-version }}-${{ steps.beam.outputs.otp-version }}-plt
          path: priv/plts
      - name: Compile code
        run: mix compile
      - name: Check code format
        run: mix format --check-formatted
      - name: Run credo
        run: mix credo --strict --all
      - name: Run dialyzer
        run: mix dialyzer --format github
      - name: Run tests
        run: mix test
...

Now, my dialyzer setp last 4s and it outputs PLT is up to date!

@Nezteb
Copy link
Sponsor Contributor

Nezteb commented May 15, 2023

he added the Gitlab example relatively recently and it uses mix.lock in the _build cache but not the PLT cache.

Yeahhh, there aren't really any better keys that I can think of unless you use every single source file in a project as part of the key. At first glance I think that'd be slow and not a good use of a build cache. 😅 In an ideal world, these build artifacts (including the PLTs) aren't changing that much between branches, so that shouldn't be necessary.

EDIT: Removed the stuff I got wrong.

I'll also ping @akasprzok to verify since he wrote the original blog post that led to me to update the GitHub Actions example and create a new one for GitLab CI. 😄

EDIT: I'm wrong, because plt_file is the project PLT, not the Erlang/Elixir PLT 🤦‍♂️. After rereading the dialyxir docs, we could probably update the CI portion to suggest using:

dialyzer: [
    plt_core_path: "priv/plts/core.plt",
    plt_local_path: "priv/plts/local.plt"
]

Instead of:

dialyzer: [
    plt_file: {:no_warn, "priv/plts/dialyzer.plt"}
]

I'm currently traveling but I'll take another look at this when I'm back!

@jeremyjh
Copy link
Owner

@Nezteb I'm thinking just adding mix.lock to the file key on the dialyzer step might do it:

@Nezteb
Copy link
Sponsor Contributor

Nezteb commented May 16, 2023

@jeremyjh Ah that makes sense.

Would it also be worth getting rid of the plt_file example since it's technically deprecated?

I can start a PR later today. 😄

@jeremyjh
Copy link
Owner

plt_file is not deprecated for usage in CI, just for local development; , or at least that is why it supports :no_warn. There is some discussion in this old issue on why plt_file is supported this way and why some may prefer it; I think otherwise you have to cache your mix home folder or put core files in _build but this may just be a matter of preference that's been replicated.

@akasprzok
Copy link

Thanks for the ping!

The problem I kept running into was that I'd push a change, and dialyzer would fail 9 minutes later (bigish code base). Then I'd push the fix, and have to wait another 9 minutes for all green.

Simply getting rid of that second 9 minutes was pretty big for me.

Adding mix.lock to the key does make sense I think.

@luigi-discoup
Copy link
Author

Simply adding the mix.lock to the key IMHO is incorrect. As far as I understand, the mix dialyzer --plt generates only core PLT files (so a basic set of OTP applications, as well as all of the Elixir standard libraries) thus is correct to bind that step to only OTP and Elixir version. Correct me if I'm wrong.

@Nezteb I made some test and plt_file: {:no_warn, "priv/plts/project.plt"} and plt_local_path: "priv/plts/project.plt" seems to have different behaviour.
The former will generate the file priv/plts/project.plt while the latter generates the folder priv/plts/project.plt/ within which there are generated PLT files. The same goes for plt_core_path, it should point to a folder, not a file.

Furthermore, it's better to store the cache at the end too, in order to cache project-level PLT.

My proposal is to use the cache mechanism described in the current github action example to handle only the core PLT files (the ones generated by mix dialyzer --plt), leaving the project-local PLTs inside _build folder and cache that with the simple actions/cache@v3 action.

@Nezteb
Copy link
Sponsor Contributor

Nezteb commented May 16, 2023

As far as I understand, the mix dialyzer --plt generates only core PLT files

@luigi-discoup I tested this, and... it depends. 😅

1. plt_file followed by mix dialyzer --plt

With this config;

dialyzer: [
    plt_file: {:no_warn, "priv/plts/project.plt"}
]

Running rm -rf _build && rm -rf priv/plts && mix dialyzer --plt produces these PLTs:

❯ tree priv 
priv
├── plts
│   ├── project.plt
│   └── project.plt.hash

And there are no PLTs in _build.

2. plt_core_path followed by mix dialyzer --plt

With this config;

dialyzer: [
    plt_core_path: "priv/plts/core"
]

Running rm -rf _build && rm -rf priv/plts && mix dialyzer --plt produces these PLTs:

❯ tree _build/dev 
_build/dev
├── dialyxir_erlang-25.1.1_elixir-1.14.1_deps-dev.plt
├── dialyxir_erlang-25.1.1_elixir-1.14.1_deps-dev.plt.hash
❯ tree priv 
priv
├── plts
│   ├── core
│       ├── dialyxir_erlang-25.1.1.plt
│       └── dialyxir_erlang-25.1.1_elixir-1.14.1.plt

3. plt_core_path and plt_local_path followed by mix dialyzer --plt

With this config;

dialyzer: [
    plt_core_path: "priv/plts/core",
    plt_local_path: "priv/plts/local"
]

Running rm -rf _build && rm -rf priv/plts && mix dialyzer --plt produces these PLTs:

❯ tree priv 
priv
├── plts
│   ├── core
│   │   ├── dialyxir_erlang-25.1.1.plt
│   │   └── dialyxir_erlang-25.1.1_elixir-1.14.1.plt
│   └── local
│       ├── dialyxir_erlang-25.1.1_elixir-1.14.1_deps-dev.plt
│       └── dialyxir_erlang-25.1.1_elixir-1.14.1_deps-dev.plt.hash

And there are no PLTs in _build.


For each set of config, I see the following available CI caching options:

  1. Cache priv/plts based on mix.lock and Erlang/Elixir versions.
    • Core PLTs would be generated from scratch on every CI run.
    • Project PLTs would be cached and updated as needed.
  2. Cache _build based on mix.lock and cache priv/plts based on Erlang/Elixir versions.
    • Core PLTs would be cached and only change with Erlang/Elixir version changes.
    • Project PLTs would be cached and updated as needed.
  3. Cache priv/plts/core based on Erlang/Elixir versions and priv/plts/local based on mix.lock.
    • Core PLTs would be cached and only change with Erlang/Elixir version changes.
    • Project PLTs would be cached and updated as needed.

The current CI examples are using option 1, while options 2 and 3 seem more ideal. The only difference between them would depend on if you want to cache your project PLTs alongside your regular build or separately in their own directory.

Thoughts?

@luigi-discoup
Copy link
Author

luigi-discoup commented May 17, 2023

@Nezteb great job! Thank you. I've only tested configuration 1 and 2, and my current CI implements scenario 2.

For the configuration 2, you said

And there are no PLTs in _build.

but it seems that there actually are PLTs in _build, right?

I think that configuration 2 or 3 can fulfill different needs, but the 2 is the easier to implement since you have to configure only plt_core_path and use only 2 separate caches configuration, one for core PLTs and one for the _build.

With the configuration 3 you have to set both plt_core_path and plt_local_path, cache core PLTS, local PLTS and _build folder, although in github action you can specify multiple path in you cache configuration with:

...
- name: Build cache
        uses: actions/cache@v3
        with:
          path: |
            deps
            _build
            priv/plts/local
          key: ${{ runner.os }}-mix-${{ hashFiles('**/mix.lock') }}
          restore-keys: ${{ runner.os }}-mix-
...

maybe the 3 is more explicit?

Anyway, I vote for the 2 :)

@Nezteb
Copy link
Sponsor Contributor

Nezteb commented May 17, 2023

For the configuration 2, you said

And there are no PLTs in _build.

but it seems that there actually are PLTs in _build, right?

Whoops yeah that was just a bad copy-paste on my part. I edited the comment. 😅

Anyway, I vote for the 2 :)

I'm inclined to agree! I'll update my docs PR to reflect that. 😄

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

No branches or pull requests

4 participants