dub icon indicating copy to clipboard operation
dub copied to clipboard

Avoid deprecation messages when calling `dub lint`.

Open veelo opened this issue 3 years ago • 12 comments
trafficstars

When the frontend version is at least 2.097, which deprecated the body keyword, require a newer dscanner version.

Fixes #2241.

veelo avatar May 31 '22 22:05 veelo

A test would involve dub removeing dscanner and all packages that it depends on, then dub fetch [email protected] (which is now the minimum that works with today's frontend, but could become too old when new language deprecations come), calling dub lint (which then goes on the Internet to reinstall these packages), capturing the output and greping for deprecation messsages. When that test starts failing: apart from Dub's code the test itself also needs amending.

I feel that this is a bit invasive, and flaky, but it can be done if so desired. Opinions?

veelo avatar Jun 01 '22 06:06 veelo

On the other hand, it is almost certain that this problem will resurface at some time in the future. So a test would be nice, I'm just not certain that we want to live with the weight of such a test until then...

veelo avatar Jun 01 '22 06:06 veelo

Switching to draft, checking out some options for a test.

veelo avatar Jun 01 '22 08:06 veelo

So as it turns out, a test is indeed useful, as you'll see it fail. Unfortunately there is a chance that pending updates to dscanner and ddox or their dependencies will clog the CI for Dub, like https://github.com/MartinNowak/hyphenate/pull/5 does now. I see there are deprecations in vibe-d also, when the latest compiler is used.

~~==> That is, if failing tests would actually turn the CI red! What's wrong?~~ My bad, had forgotten the return value.

The alternative is to always upgrade these tools on every invocation like #1075 suggests, but there is recent sentiment that Dub should not go on the Internet all the time.

veelo avatar Jun 01 '22 13:06 veelo

So as it turns out, a test is indeed useful, as you'll see it fail.

Thanks for that. Let me know if there is anything you think that I can do to help push this forward, otherwise ping me when this is in a state to be merged.

thewilsonator avatar Jun 02 '22 11:06 thewilsonator

Well, frankly I don't think this will work at all, or rather, it works way too well. I don't think it is good that missing updates in distant dependencies (not code dependencies, but dependencies of tools that Dub may fire up) will block PRs to Dub from turning green; PRs with changes that are completely unrelated to these dependencies. Since dmd-master is in the Testsuite, that will happen as soon as a deprecation is committed to dmd, before packages have had a chance to resolve them. An example is the tons and tons of dip1000 deprecations in vibe-d at this moment.

I have been looking for a way for a test to succeed with warnings, but there doesn't seem to be one. A test either fails or passes.

Is there a way to run a GitHub action so that it won't block merging, only raise awareness? A second workflow maybe?

veelo avatar Jun 02 '22 12:06 veelo

I have been looking for a way for a test to succeed with warnings, but there doesn't seem to be one. A test either fails or passes.

The default is for dub to turn warnings into errors. Perhaps we need a switch to turn this off?

rikkimax avatar Jun 02 '22 12:06 rikkimax

I have been looking for a way for a test to succeed with warnings, but there doesn't seem to be one. A test either fails or passes.

The default is for dub to turn warnings into errors. Perhaps we need a switch to turn this off?

This is not about a warning from the compiler causing a compilation to fail. This test is designed to find deprecation messages in a compilation (not of Dub but of tools that it uses). So the issue is the return value of the test. We want the test to make some noise but not block merging of the PR in question.

It is actually a test that is not geared towards finding flaws in a PR. It is meant to run from time to time, probing the tools, and when it fails then action does not necessarily need to be taken right away, only when tools release a new version that fixes the deprecation.

I could just let the test log a warning and succeed, but nobody will be checking the logs for a test that is green.

veelo avatar Jun 02 '22 13:06 veelo

Let's go back to first principles.

External packages

Dub uses some external packages for some of its commands. When the package is available locally, it may

  1. Not work because it is too old (https://github.com/rejectedsoftware/ddox/issues/139) , and
  2. Show deprecation warnings when Dub for some reason decides that it needs to be recompiled (https://github.com/dlang-community/dsymbol/issues/171). This is not a real problem, it is just that users don't like seeing them and think it is Dub's fault.

Thus the performance of Dub depends on local factors such as the package cache and the compiler frontend version.

Users can resolve these issues by fetching a different version of the package, but this is in no way apparent and they should not need to know that dscanner is used for dub lint.

Let's see what our options are.

Knowledge base

This is the approach taken by this PR: Somehow maintain the knowledge about which package versions work for which compiler frontend versions. This is a bad solution because

  1. It needs constant maintenance.
  2. There is no way to know whether the knowledge is complete (and it probably won't ever be).
  3. There may still be deprecation warnings.

Always look for the newest version

As suggested in #1075. This has some disadvantages:

  1. Going on the Internet al the time.
  2. May not work if the user is tied to an older compiler frontend version and the latest package uses a new language feature.
  3. Users have no control over the version being used.
  4. Breaks reproducible builds: Suddenly the layout of the generated documentation may be different from one build to the next.

Distribute binaries

When Dub would distribute and use these packages in binary form (either as stand alone executable or used as a library) it would not involve compilation on the user's system, and hence that step could not fail or show deprecation messages. But:

  1. Users would not be able to control the version of the tools, for better or for worse.
  2. Complicates distribution. Would need to be included by everything that includes Dub (compiler distributions).
  3. Dub is a package manager, but it cannot manage these...

Build-dependencies

Dub could have the concept of build dependencies. A build dependency is a package that is executed in the build process, like dpp. Build dependencies are not linked into the target, but they do affect the target and therefore should be versioned. ddox is a build dependency, considering documentation is an asset. dscanner also fits in this category, as projects may want to control the version of dscanner being used. Advantages:

  1. Users would have full control over versions in a way they are familiar with.
  2. No excess Internet access. dub upgrade would upgrade build dependencies alongside normal dependencies.
  3. When dub lint or dub build -b ddox is called without a respective build dependency being present in the Dub configuration, Dub could either inform the user what to do or propose to do it for them.
  4. Users will still see deprecation messages, but it will be clear that they stem from a dependency, not from Dub. deprecated is a feature, not a bug.

(Actually, the compiler is a build dependency also, and it would be practical if Dub could just go and get the proper version instead of halting like it does when toolchainRequirements are not met. But that is another subject.)

veelo avatar Jun 02 '22 15:06 veelo

you could consider making another GitHub Actions CI file that runs periodically (not on PRs or commits) and add a badge to the README for others to know quickly if tools are outdated. In the case of test failure there, emails are also sent to maintainers. (like they would in regular commit fails on the master branch)

WebFreak001 avatar Jun 03 '22 07:06 WebFreak001

I agree it is hacky and not optimal.

It just occurred to me that the root cause is that dub run considers a recompilation.

  1. Dub has cached an older version of the tool.
  2. The compiler was upgraded in the mean time.
  3. Dub concludes that the tool needs to be recompiled. This may fail or succeed with deprecation messages. It doesn't try an upgrade.
  4. The tool is executed.

But step 3 is not necessary in our scenario. If the tool was cached, it can just execute the cached executable. The reason for dub run to include a full dub build is probably because it was meant as a shorthand during development of a package. But you don't want that if the package is just a tool and you want to just use it. Tools should be upgraded as part of dub upgrade.

So I think we should consider adding an option like dub run --cached, which skips the build step if an executable exists in the cache. Thoughts?

veelo avatar Jun 12 '22 11:06 veelo

Alternatively, if Dub decides that the cache is dirty, it could do an upgrade of the tool before building. This way tools are upgraded in a reasonable tempo, without checking the Internet on every invocation. This is unlikely to break reproducible builds or cause issues for people that are tied to older frontends. Users can enforce using an older version of a tool by dub build tool@version manually.

veelo avatar Jun 12 '22 11:06 veelo