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

Upgrade to tracing and color-eyre #602

Closed
wants to merge 14 commits into from

Conversation

ActuallyHappening
Copy link
Contributor

@ActuallyHappening ActuallyHappening commented Feb 6, 2025

These changes make errors go from this:
image
To this:
image

And logs from this:
image
To this:
image

Breaking changes:

Maybe anyhow was accidentally exposed somewhere? color-eyre was not exposed anywhere that anyhow wasn't, so for library usage this is a breaking change
The time format for local offsets doesn't cleanly continue and instead errors on unable to find local UtcOffset
Also, the default logging implementation used only logs to stderr, I don't know if there was some logic in the old implementation to send some logs to stdout and some to stderr, but its now all consistent to stderr

Checklist

Please make sure the PR adheres to this project's standards:

  • I included a new entry to the CHANGELOG.md.
  • I checked cargo clippy and cargo fmt. The CI will fail otherwise anyway.
  • (If applicable) I added tests for this feature or adjusted existing tests.
  • (If applicable) I checked if anything in the wiki needs to be changed.

@Nukesor
Copy link
Owner

Nukesor commented Feb 6, 2025

Heyo,

First of all thanks for the contribution!

Second thing: When doing such large scale refactorings, please talk with maintainers first. Especially when introducing architectural changes such as a prelude. This will take some time to review, and there might be some things that upstream doesn't like, potentially resulting in loads of your changes needing to be reverted. This is just a general piece of advice to make stuff like this less frustrating for both sides :)

That being said, I'll try to review this soonish, there's a lot of stuff going on right now so I'm not sure when I'll make it :)

@ActuallyHappening
Copy link
Contributor Author

All good, I'm using my version in production already, hopefully the colourful error messages can get to everybody :)

Also, one thing, I'm connecting over TCP/TLS to my remote server, but its erroring because the pueue add is sending up the local CWD path since I'm not explicitely passing the --working-directory flag, is there a clean way to avoid this CLI arg necessity? I can add that, although probably in a seperate issue, E.g. a config option to disable the default in my custom config to connect to the remote.
Anyway, pueue is an amazing tool to work with, great documentation which has helped a lot :)

@ActuallyHappening
Copy link
Contributor Author

About the prelude, its completely private and only an internal module (note the pub(crate) visibility), this pattern is excellent for enforcing consistent project-wide invariants like error handling, with a prelude the entire refactor would have been as simple as changing one or two prelude modules to point to the new library.

@Nukesor
Copy link
Owner

Nukesor commented Feb 7, 2025

Regarding the first issue of the CWD.

If the CWD on the local isn't being used, what CWD should then be used instead. The only reasonable thing I can come up with is /, but in that case all paths would need to be absolute as well, so I don't see a big advantage?
Ah, or is it because the dir doesn't exist on the remote and the process cannot start because of that? Damn, I haven't thought about that....
Yeah that would be a separate issue.

Regarding the prelude:
I can kind of see that. I find the name prelude still a bit off as that's usually a pattern used to expose stuff externally.
And something resists in me against this pattern :D. This is something i need think through for a few days to figure out why that is. Might change my mind while doing so :)

@ActuallyHappening
Copy link
Contributor Author

Yeah all good, specifically for error handling I also like to have an (internal in this case) crate called errors that I (internally in this case) reexport from the prelude, e.g.

pub(crate) mod prelude {
pub(crate) use whatever_global_lib::prelude::*;
pub(crate) use crate::errors::*;
}

pub(crate) mod errors {
pub type Result<T> = color_eyre::Result<T>;
pub type Error = color_eyre::Report;
}

These internal patterns I have devised and used because they have proven their worth to me over many wildly varying projects, from embedded_hal trait exports to higher-level Utf8PathBufExt extension traits. They are internal only unless you don't want them to be, and the way I implemented them in this PR was as conservative as possible (I would personally re-export std::path::Path and std::path::PathBuf probably because you imported it many times across many files).
The only downside I can see to the prelude is increased ambiguity in type names, now reading fn foo() -> Result<T> it may not be as obvious what crate Result<T> comes from if you don't name the crate, use color_eyre::Result, but the upside is you can now switch back to anyhow within a minute of refactoring rather than ten.

@ActuallyHappening
Copy link
Contributor Author

ActuallyHappening commented Feb 7, 2025

About the / CWD thing, all of my paths (unless hand crafted) were using absolute paths anyway, so I made an alias padd = pueue add --working-directory / which solved that problem. This PR also adds a few sanity checks for future explorers, there is now a warn!() if the current-dir path doesn't exist (on the daemon side) instead of a generic error, although this will be hidden if you pueued -d I'm pretty sure (*hidden meaning its printed to stderr, but you may not immediately know where those logs go)

Copy link

codecov bot commented Feb 8, 2025

Codecov Report

Attention: Patch coverage is 59.34066% with 37 lines in your changes missing coverage. Please review.

Project coverage is 80.41%. Comparing base (30359aa) to head (f41b295).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
pueue/src/tracing.rs 60.00% 16 Missing ⚠️
pueue/src/client/client.rs 11.11% 8 Missing ⚠️
pueue/src/daemon/process_handler/spawn.rs 58.82% 7 Missing ⚠️
pueue_lib/src/process_helper/mod.rs 66.66% 5 Missing ⚠️
pueue/src/client/cli.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #602      +/-   ##
==========================================
- Coverage   80.89%   80.41%   -0.49%     
==========================================
  Files          75       76       +1     
  Lines        5942     5959      +17     
==========================================
- Hits         4807     4792      -15     
- Misses       1135     1167      +32     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

github-actions bot commented Feb 8, 2025

Test Results

  2 files   -  1   14 suites   - 5   3m 29s ⏱️ +4s
173 tests  -  1  173 ✅  -  1  0 💤 ±0  0 ❌ ±0 
346 runs   - 20  346 ✅  - 20  0 💤 ±0  0 ❌ ±0 

Results for commit f41b295. ± Comparison against base commit 30359aa.

This pull request removes 1 test.
pueue-lib ‑ process_helper::platform::test::test_spawn_command

♻️ This comment has been updated with latest results.

Copy link
Owner

@Nukesor Nukesor left a comment

Choose a reason for hiding this comment

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

Nice.

Generally, the logs look much more neat and I really appreciate the work :)

There're a few general issues:

  1. You drive-by bumped the dependencies of the project in a completely unrelated commit. Please do that in a separate commit or even in a separate MR, it makes it easier to review, dissect, debug and revert.
    If rebasing the history is too annoying for you, I would be ok with squashing everything into a single commit to make this easier.
  2. FYI The import formatting isn't really to my liking. We now have a crate import at the very top, followed by std imports, external lib imports, pueue_lib imports, followed by crate imports again.
    That being said, I'll run a cargo fmt with some nightly options as soon as this is merged, so this will be fixed later on by me. Just wanted to give you a heads-up that this will change in the future.
  3. I'll try to be open-minded and see how the prelude works out :D
    One easy-to-fix thing though, please rename prelude to internal_prelude to prevent confusion. The prelude pattern is widely known and the expectations to new developers will be that it exposes stuff to library users (which was my first expectation as well). Having a different name should prevent this.

pueue/src/daemon/process_handler/mod.rs Outdated Show resolved Hide resolved
pueue/src/lib.rs Outdated Show resolved Hide resolved
pueue/src/lib.rs Outdated Show resolved Hide resolved
pueue/tests/daemon/integration/edit.rs Outdated Show resolved Hide resolved
pueue/Cargo.toml Outdated Show resolved Hide resolved
pueue/src/lib.rs Outdated Show resolved Hide resolved
pueue/tests/helper/mod.rs Outdated Show resolved Hide resolved
pueue_lib/src/network/tls.rs Show resolved Hide resolved
pueue_lib/src/process_helper/mod.rs Outdated Show resolved Hide resolved
pueue_lib/src/process_helper/mod.rs Outdated Show resolved Hide resolved
@ActuallyHappening
Copy link
Contributor Author

Ok I've committed my changes for everything bar the unresolved requested changes above, tbh I'm not to confident with git and don't know how to rebase the cargo update I ran, there is not too much value in preserving each of my incremental commits so I'm fine for them all to be squashed if that helps

@Nukesor
Copy link
Owner

Nukesor commented Feb 9, 2025

Yeah rebasing into sensible commits would be annoying :D

One easy way to do this would be the following:

git reset --soft main
git checkout main Cargo.lock 
cargo build
# Do a new commit

That should squash the changes and only introduce the Cargo.lock changes of your remove/added dependencies
When you've squashed the changes, just commit --amend changes afterwards.

@Nukesor
Copy link
Owner

Nukesor commented Feb 9, 2025

I just noticed that you don't work on a feature branch. Well, this complicates things.
You did some form of rebase or cherry-pick on my main branch and borked your history while at it.

Please reset to the latest branch point (which would be 30359aad), switch to a proper feature branch and make a clean commit. I'm not sure if you can change the source branch in git, if not just create a new PR :D.
Also try to get my two commits out of your commit history in some way. A rebase onto 30359aad without picking those commits would probably work just fine.

In case you're interested, here's my usual Git Workflow

Your main should always mirror upstream's main, otherwise the git history gets seriously messy!
I.e you're currently shadowing some changes I did in the meantime.

image

@Nukesor
Copy link
Owner

Nukesor commented Feb 9, 2025

Also check the Github Action pipelines.

The Windows code breaks as you didn't adjust the code over there.
An easy way to iterate and debug windows/MacOs changes would be to create a merge request against your own repository's main, which will then run the pipelines over there.

Over here you have to wait for me to approve the pipeline first.

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.

2 participants