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

feat: Adding additional logging and configuration #11

Merged
merged 18 commits into from
Mar 1, 2024

Conversation

CEbbinghaus
Copy link
Owner

To aid in troubleshooting bug reports more information will be required. To this end the log crate is being replaced with trace which is a lot more powerful. A toml configuration file is also added to allow configuration of the backend, including the level the logs should be written as.

@jfreuden
Copy link
Contributor

jfreuden commented Feb 9, 2024

It seems that moving the version file angers the backend compilation in the CI/CD github pipeline. Got this from lines 494-504 of the build log

     Compiling backend v0.0.0 (/backend)
  error: couldn't read src/../version: No such file or directory (os error 2)
   --> src/env.rs:5:43
    |
  5 | pub const PACKAGE_VERSION: &'static str = include_str!("../version");
    |                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: this error originates in the macro `include_str` (in Nightly builds, run with -Z macro-backtrace for more info)
  
  error: could not compile `backend` (bin "backend") due to previous error
  mv: cannot stat 'target/docker/backend': No such file or directory

I confirmed that it didn't do this previously here

@CEbbinghaus
Copy link
Owner Author

@jfreuden REALLY good catch. Wow I cannot believe the action passed even though it threw. I investigated it and it looks like Docker is once again the problem, Second being the fact that the Decky build tool requires the docker command to be run in a specific way. I think in future I will want to expand the tool to allow arbitrary binds (so I can just mount the file in the right location) but for the moment it looks like I will have to once again swap them around so the "original" is in the backend folder...

I really wish that docker let you copy a file from outside its directory :/

@CEbbinghaus CEbbinghaus added this to the 1.0.0 milestone Feb 16, 2024
@CEbbinghaus CEbbinghaus added bug Something isn't working enhancement New feature or request and removed bug Something isn't working labels Feb 16, 2024
@CEbbinghaus CEbbinghaus self-assigned this Feb 16, 2024
@jfreuden
Copy link
Contributor

I saw that you fixed the version error as well as got rid of the warnings. Did you ever figure out how to ensure the Action sees a compile error of backend as an unrecoverable failure?

@CEbbinghaus
Copy link
Owner Author

Not yet. the docker run command succeeds even if the process fails. I need to find a way for the docker container to exit with the same status code as the process did.

@jfreuden
Copy link
Contributor

The build has entrypoint.sh as the... entrypoint... and in there it makes a new shell with bash which runs build-docker.sh and in that script the cargo build --profile docker line is run, but doesn't exit when it fails.
Should be able to either do a simple cargo build --profile docker || exit 1 for the hacky version or

cargo build --profile docker
BUILD_EXIT=$?
# ... rest of script then last line ...
exit $BUILD_EXIT

I'm not sure whether something similar needs to be done on the entrypoint.sh script. Maybe, maybe not. I'm gonna see if I can't test this on my fork and get you the answer in a couple of hours

@jfreuden
Copy link
Contributor

Well the good news is that my solution does make docker now see failures but the decky plugin build still doesn't care and it barges right in to the frontend build. oh well. I'm pretty sure I have a project lying around somewhere that had a similar issue a long time ago. I'll keep looking for a solution as well but I'm wondering if it's a decky problem we can't solve here...

@CEbbinghaus
Copy link
Owner Author

Wonderful, Thank you so much for helping fix that <3

@jfreuden
Copy link
Contributor

Wonderful, Thank you so much for helping fix that <3

I put a PR pointing to this branch, so that as you keep working on this PR the actions will only take a CLI update in order to show accurate runner statuses. I can also cherry pick it to point wherever, it doesn't really matter where it goes 😄

* Change the build-docker.sh file to return exit code from build
According to rust-lang/cargo#3377 (comment)
the cargo search doesn't update the index and later in the thread is pointed out this is unnecessary now. Which is good because this doesn't work anymore
* Version bump of Decky CLI to 0.0.2 should fix the Github Runner failing to register error status codes from `cargo build` in backend
Copy link
Owner Author

@CEbbinghaus CEbbinghaus left a comment

Choose a reason for hiding this comment

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

Lets hope I didn't miss anything :3

@CEbbinghaus CEbbinghaus merged commit 7248d7a into master Mar 1, 2024
2 checks passed
@CEbbinghaus CEbbinghaus deleted the feat/FixingProblems branch December 17, 2024 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants