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

Added support for xterm 256 colors and 24 bit colors #96

Merged
merged 4 commits into from
Mar 17, 2017

Conversation

JoeMerten
Copy link
Collaborator

This PR adds support for esc[38;…m / esc[48;…m for 256 colors and 24 bit foreground / background colors. See also:

Please note, that this needs jansi PR #72 to build & work. I'd increased the jansi version dependency in the pom.xml to 1.14-SNAPSHOT to build locally.

Further, we run into a version bump as described in #95 which leads also to make 256 colors and 24 bit colors don't work (even if we have successfully build and all plugin tests passed).

As I found a way to got it work with Jenkins 1.653 (by just deleting jansi-1.9.jar), I'd found no way to make it work with Jenkins 2.x. As dblock commented in #95, we might have to wait until even the Jenkins guys will pick up the latest jansi changes.

@@ -88,7 +88,7 @@
<dependency>
<groupId>org.fusesource.jansi</groupId>
<artifactId>jansi</artifactId>
<version>1.12</version>
<version>1.14-SNAPSHOT</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to ship with an unshipped version of 1.14. I am not saying we shouldn't merge this, but we should be careful releasing with this.

@dblock
Copy link
Member

dblock commented Mar 8, 2017

... and this doesn't pass build since the SNAPSHOT version is a private one.

@JoeMerten
Copy link
Collaborator Author

Ok, then I just have to wait for:

Both might take a indefinite period of time.

And after this, I'll:

  • change the pom.xml in this PR.
  • send a PR to jenkinsci to also change their jansi version in pom.xml

CU next time, Joe.

@dblock
Copy link
Member

dblock commented Mar 10, 2017

Thanks Joe!

Maybe release what's on master though? It fixes a number of issues.

@JoeMerten
Copy link
Collaborator Author

In #95, you commented:

… but maybe including JANSI as a built submodule (in a different namespace) is OK?

Could you help me to understand the idea behind this approch?
Then I'll give it a try to make MoreColors work without waiting for Jansi & Jenkins repos (and I could refactor the code as soon as Jansi & Jenkins have been caught up).

@dblock
Copy link
Member

dblock commented Mar 15, 2017

I mean checking out the source of Jansi as a git submodule, then bulk replacing the namespace by something internal during the build and using that code.

JoeMerten pushed a commit to JoeMerten/jenkins-ansicolor-plugin that referenced this pull request Mar 16, 2017
Embedded jansi's AnsiOutputStream.java into our source tree to avoid
version bumping. See also jenkinsci#95 and jenkinsci#96.
@JoeMerten JoeMerten mentioned this pull request Mar 16, 2017
@JoeMerten
Copy link
Collaborator Author

I'm a bit aware about git submodule because of trouble in the past (but however, I'm not git expert at all).
What about PR #98? I'd tested it even in bleeding edge jenkins 2.50 and it works as expected.

@dblock dblock merged commit cc0fa81 into jenkinsci:master Mar 17, 2017
@dblock
Copy link
Member

dblock commented Mar 17, 2017

I like it enough. Party on.

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