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

#3140: Save error message #3185

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

levBagryansky
Copy link
Member

@levBagryansky levBagryansky commented May 12, 2024

Closes #3140


PR-Codex overview

The focus of this PR is to enhance the Rust build process in the Maven plugin by introducing new features and improvements.

Detailed summary

  • Added WeHaveCargo as an execution condition for Rust builds
  • Modified exception handling in BuildFailureException
  • Updated Rust build process in RustNode class
  • Added new tests for Rust build failures and error messages

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@levBagryansky
Copy link
Member Author

@maxonfjvipon WDYT

@levBagryansky levBagryansky changed the title #3140: Call command without Jaxec #3140: Save error message May 13, 2024
@levBagryansky
Copy link
Member Author

@maxonfjvipon please check

Copy link
Member

@maxonfjvipon maxonfjvipon left a comment

Choose a reason for hiding this comment

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

@yegor256
Copy link
Member

yegor256 commented May 13, 2024

@levBagryansky did you try to use .withRedirect(true) of Jaxec, as explained here?

@levBagryansky
Copy link
Member Author

@yegor256 Yes, it still did not print it.

@yegor256
Copy link
Member

@levBagryansky we should find out, why it doesn't print. Most probably, something is wrong with the configuration of logging levels in log4j.properties file. Just throwing away the library and re-implementing its functionality is not a good practice.

@levBagryansky
Copy link
Member Author

@yegor256 log4j.properties are configured right, and Jaxec has error message inside, in result.stdout(). The problem is that this message is not attached to Exception in line https://github.com/yegor256/jaxec/blob/master/src/main/java/com/yegor256/Jaxec.java#L365 so we cannot get it.

@yegor256
Copy link
Member

@levBagryansky why it's important to put entire stdout into an exception (pretty unusual practice) instead of just sending it to the console/log? What do you mean by "we cannot get it"? If you can see the stacktrace (printed to stderr), you should see the log printed to stdout, I believe.

@levBagryansky
Copy link
Member Author

@yegor256 because usually we would not like to see logs for all cargo projects. There are several inserts in eo program so logs from them would be mixed.

@levBagryansky
Copy link
Member Author

@yegor256 We would like to get stdout & stderr of the process as String as the result of Jaxec::execUnsafe. Then we would print this String to log. But the problem is that execUnsafe does not return the String in case of exception, and it does not wrap it to Exception.
Looks like the only way to use Jaxec is to print stdout continuously. But then the log will be too unreadable, since many cargo processes will print their stdout at the same time.

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.

Print full error message if cargo fails
3 participants