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 JavacLogger, for #68 #74

Merged
merged 1 commit into from
Jun 25, 2011
Merged

Added JavacLogger, for #68 #74

merged 1 commit into from
Jun 25, 2011

Conversation

duboisf
Copy link
Contributor

@duboisf duboisf commented Jun 24, 2011

Had commited to 0.10 branch, created a new topic branch for the pull request.

Like I said previously, if I don't use var exitCode = -1 how can I pass it into javacLogger.flush? As in:

try { Process... } finally { javacLogger.flush(???) }

So I used this instead:

allCatch opt { Process... } match { 
  case exitCode => {
    javacLogger.flush(exitCode)
    exitCode getOrElse (-1)
  }
}

@harrah
Copy link
Member

harrah commented Jun 25, 2011

I'm sorry about that. I missed you saying that the first time and didn't catch it myself either time (obviously). Your original code with the var was good, so let's go with that one. I don't think you need to re-fork, just create a new branch. You can delete the branch after I merge it if you want. Finally, please confirm new BSD license is ok.

- JavacLogger.msgs uses ListBuffer for constant time append
- Synchronized access to JavacLogger.msgs since appends comes from multiple
  threads
- JavacLogger.info uses Level.Info instead of Level.Debug
- Wrapped call to javac in allCatch to guarantee logger being flushed
@duboisf
Copy link
Contributor Author

duboisf commented Jun 25, 2011

No worries.

Yeah I reforked because for some reason I couldn't delete the 0.10 branch in my fork and I wanted to rebase it. From now on I'll always do topic branches first :)

BSD license is ok. Just out of curiosity, what was it before?

One final note, I forgot to change logging from Debug to Info in the print function:

case (Info, msg) => log.debug(msg)

so I changed it to

case (Info, msg) => log.info(msg)

harrah added a commit that referenced this pull request Jun 25, 2011
@harrah harrah merged commit 48940bf into sbt:0.10 Jun 25, 2011
@harrah
Copy link
Member

harrah commented Jun 25, 2011

The name of the license is "new BSD". The license is same as before. Thanks again for the patch!

@duboisf
Copy link
Contributor Author

duboisf commented Jun 25, 2011

I rebased my modifications in my issue_68 branch into a single commit and deleted and repushed the issue_68 branch.

Github didn't pick up that it has changed (well it says that it was 2 days ago) but if you look at the commit above (2f4be49) it's the one I just updated.

Sorry, new to github's pull requests, if I'm not doing it right, do tell :)

@duboisf
Copy link
Contributor Author

duboisf commented Jun 25, 2011

My pleasure!

@harrah
Copy link
Member

harrah commented Jun 25, 2011

Yeah, it's merged now, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants