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

Warnings and Depwarns are not detected as warnings inside GitHub actions #36326

Closed
aminya opened this issue Jun 17, 2020 · 17 comments
Closed

Warnings and Depwarns are not detected as warnings inside GitHub actions #36326

aminya opened this issue Jun 17, 2020 · 17 comments

Comments

@aminya
Copy link

aminya commented Jun 17, 2020

GitHub-actions allows the code to throw warnings. However, Julia's warnings are ignored.

Examples of warnings that are detected by GitHub actions (JavaScript code):
https://github.com/JunoLab/atom-indent-detective/actions/runs/97242509

We should either fix this from the Julia side, or make an issue on the GitHub side

@christopher-dG
Copy link
Member

This is unrelated to Julia itself, if you want your warnings picked up you need to use a custom logger. I did that for TagBot, for example (although it's Python).

@aminya
Copy link
Author

aminya commented Jun 17, 2020

In JavaScript, all the warnings are detected by default. Why don't we fix this for Julia too? My code is fully written in Julia. No python or JavaScript.

@giordano
Copy link
Contributor

To complement what Chris said, this is the format of the messaages: https://help.github.com/en/actions/reference/workflow-commands-for-github-actions.

In JavaScript, all the warnings are detected by default. Why don't we fix this for Julia too?

Why to change the logging messages for the entire ecosystem to make GitHub Actions happy? Why not using the format required by Azure Pipelines? Why not the format by X CI service instead?

@christopher-dG
Copy link
Member

I don't think it's "in JavaScript", I think it's that eslint identifies the GHA environment and therefore emits logs in that format.

@StefanKarpinski
Copy link
Sponsor Member

What would we do on the Julia side to fix this?

@aminya
Copy link
Author

aminya commented Jun 17, 2020

We can introduce a function that detects the environment and throws the correct format of the warning. For example, the format required for GitHub Actions, Azure, Travis, etc.

We can also add a keyword argument to the current warning functions to optionally call that CI warning for us. 🤔

@giordano
Copy link
Contributor

In Yggdrasil we have a custom logger which duplicates the logging messages, to appease Azure Pipelines, which have a very funky unreadable format: https://docs.microsoft.com/en-us/azure/devops/pipelines/scripts/logging-commands?view=azure-devops&tabs=bash.
This is the custom logger for Yggdrasil: https://github.com/JuliaPackaging/BinaryBuilder.jl/blob/b13d66169cf84fc1e1193ebbc0fb16910c9a6f34/src/Logging.jl

@christopher-dG
Copy link
Member

christopher-dG commented Jun 17, 2020

If I were wanting this, I'd probably add a new logger to LoggingExtras or similar and use that in my tests. If I were a programming language maintainer, I wouldn't want to have to keep up with various CI service specifics in my standard library.

@aminya
Copy link
Author

aminya commented Jun 17, 2020

I would not mind if it becomes a third-party package. The disadvantage of that will be that LoggingExtras would then be the dependency of every package that throws some warnings.

Instead, if the language itself fixes this issue, people will not need to do that. But that is only my opinion.

@christopher-dG
Copy link
Member

Not everyone uses GitHub Actions though. I'd love for Julia to trend towards less GitHub coupling rather than more, personally.

@giordano
Copy link
Contributor

The disadvantage of that will be that LoggingExtras would then be the dependency of every package that throws some warnings.

Why so? You need it only for the tests

@fredrikekre
Copy link
Member

If I were wanting this, I'd probably add a new logger to LoggingExtras or similar and use that in my tests.

👍

@aminya
Copy link
Author

aminya commented Jun 17, 2020

The disadvantage of that will be that LoggingExtras would then be the dependency of every package that throws some warnings.

Why so? You need it only for the tests

Yes. People usually run the tests on some CI environment. For extras, this is a serious issue. If people knew about the deprecations, then we did not have the failures that we see in some packages. Most of the time, people get happy when their CI pass, without reading the log.

See Plots for example:
JuliaPlots/Plots.jl#2789

@christopher-dG
Copy link
Member

I think one important distinction to make between eslint and Julia logging is that eslint is a linter, not a logging system. When it emits warnings, they are issues being found by the linter. A Julia log message with level "warning" or even "error" might be completely expected in a test suite, and I think that seeing that in your CI logs over and over would be annoying.

@giordano
Copy link
Contributor

I understand the usefulness of making warnings more visible, but I don't understand why

  1. this should be done in Julia
  2. this would need extra dependencies for your package. You can create a package called GitHubActionsLogger.jl where you define a custom logger, use it as a tests-only dependency in your packages to change the logger if you detect that you're in GitHub Actions

@aminya
Copy link
Author

aminya commented Jun 17, 2020

I understand the usefulness of making warnings more visible, but I don't understand why

  1. this should be done in Julia

Do we want to make every warning or error visible, or just some of them? I don't understand why some warnings are less important and some are more.

  1. this would need extra dependencies for your package. You can create a package called GitHubActionsLogger.jl where you define a custom logger, use it as a tests-only dependency in your packages to change the logger if you detect that you're in GitHub Actions

At least let's create one package for all of them! NOt doing that people will have to have tens of these packages to support every CI environment 😄.

@StefanKarpinski
Copy link
Sponsor Member

StefanKarpinski commented Jun 17, 2020

@aminya, this issue is following what is becoming a fairly typical pattern:

  1. You want to add some random thing to Julia in order to solve some relatively niche problem;
  2. Multiple people here take the time and energy to patiently explain why this is not the right way/place for that functionality; they provide alternative solutions.
  3. You argue and reject the proposed solutions and arguments.

I'm going to lock this issue to not waste any more time and energy. It is very much appreciated that you want to improve Julia and its ecosystem. But please respect the feedback and judgement of those here who are willing to offer feedback and alternative solutions.

@JuliaLang JuliaLang locked as resolved and limited conversation to collaborators Jun 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants