Colored output #2268
Merged
Colored output #2268
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
veelo
force-pushed
the
colored-output
branch
2 times, most recently
from
15925e2 to
8323d5d
Compare
|
Are there any Mac users with an idea why this line gives a broken pipe error? |
WebFreak001
requested changes
Jun 20, 2022
There was a problem hiding this comment.
I like this change, I think to stay consistent with colors it's probably a good idea to define some config file where we define some common colors like colorPackageName, colorPath which we use on all package names that are printed.
Additionally for the tags create a TagColor enum which has values set to the corresponding Color enum values, but with more context-specific names such as TagColor.processing, TagColor.execution, TagColor.finished
This way it's easier to adjust the colors later consistently.
In my comments here I suggested to make paths bold, to be consistent with dmd error messages, but because package names are already bold we might want to consider instead underlining or for example doing some additonal coloring with the package names.
|
What about the merge conflicts at ? |
|
Do you have time to look into these comments, @veelo? |
I'll probably be able to make time for these, but not before I know there is a solution for the failing CI on MacOS. If anybody is able to reproduce this on a Mac that would probably help a lot, but even then some questions remain. I assume the color codes are creating a problem in the terminal there, and I am not sure if that points to a problem with the implementation, or a problem with the shell script. Therefore I am hesitant of replacing the shell script with a D script, because that just might hide the issue. |
Ok. What command should be run locally on a Mac to reproduce the issues? |
It is Actually, I now see there are several other failures higher up. It appears that this is the script that runs all the tests, so try running this: |
For starters, on Ubuntu 22.04 using DMD64 D Compiler v2.100.0, git clone --recurse-submodules https://github.com/veelo/dub.git
cd dub
DC=dmd scripts/ci/travis.shfails as |
You are probably missing this dependency: |
Same behaviour after sudo apt-get install -y libcurl4-openssl-dev netcathas been run. |
|
We've skipped building dub, probably: https://github.com/dlang/dub/blob/master/.github/workflows/main.yml#L66 |
Are you saying that file is missing dub build --compiler=${{ env.DC }}somewhere? Does apply to Mac? |
|
It is not clear to me what step the link errors appear in, as you did not include that information in the output. But if you only ran the tests without building dub first, that can be an issue. So should be run before running the travis script.
Yes. |
To be clear this uses the installed |
|
I see, thanks. The command git clone --recurse-submodules https://github.com/veelo/dub.git
cd dub
dub build --compiler=dmd
DC=dmd scripts/ci/travis.shstill errors as |
|
So dub builds fine, because that step ends at the empty new line in your output. But you are not in the right branch, make sure to check out the |
Doh, will adjust. Trying git clone --recurse-submodules https://github.com/veelo/dub.git
cd dub
git checkout colored-output
dub build --compiler=dmd
DC=dmd scripts/ci/travis.shgives |
Still not the right version... |
Are you saying I'm building dub with the wrong version of dub? Now, git clone --recurse-submodules https://github.com/veelo/dub.git
cd dub
git checkout colored-output
git branch
dub build --compiler=dmd
DC=dmd scripts/ci/travis.shgives . |
Not the correct branch. |
|
If you checked out the |
I am on the right branch. I don't understand. Isn't 1.24.1-beta.1+commit.205.g96d25366 the dub version that is used to build dub? |
|
Very strange. I get this (on Windows):
That is the version currently being built. |
The test was susceptible to interference from other tests, because it assumed the 1-exec-simple project was clean, which may be not. The reason that made the test work until now was that it happened to execute before other tests that use the same project. Now it ensures the project is clean before running. This commit is part of PR dlang#1490 because it was needed in order to make the new colored-output test pass (which was interfering with this one).
(the .gitignore should be updated for people that runs tests locally...)
…ginal version of Dub
|
This PR has passed 60 commits, which makes rebasing time consuming. I have done that three or four times now. Can we give this merge priority please? Thanks! |
WebFreak001
approved these changes
Jul 28, 2022
|
few things I just found while trying this out:
@veelo do you want to fix the dub list thing before merging? |
I'll try to look into that tomorrow. |
|
ok might as well get this in now and then get feedback from users |
|
Next release is going to be 🔥 |
|
Thanks guys, glad to get this in! |
|
Does the |
|
Not sure, I cannot remember seeing that implemented. It's a good idea! |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Picking up what was started in #1490 and #2124.