remove pkg/context, overhaul snapshot handling #417
Conversation
jaypipes
force-pushed
the
context
branch
6 times, most recently
from
f3ed2e1 to
128887b
Compare
ffromani
left a comment
ffromani
left a comment
There was a problem hiding this comment.
First of all thanks for filing this! design wise I like this approach a lot and I'm all for it.
The main question I have is: how do we deal with backward compatibilty? this is an API breakage both at golang package level and at command line level (tool removal).
Perhaps we can bump the major version? would that too much of a jump, or too little?
Glancing over the PR everything seems reasonnable, but it will take me some time to ready everything (other duties :\ )
@ffromani Ciao! :) Yeah, it's a breaking change for sure, though I worked hard to minimize the changes that "normal" users would see. The call signature for all of the Info() functions remains the same so their Go code that imports the ghw library would not need to change. And the I don't think it warrants a major version bump because there's no functional change to the API besides the removal of the SnapshotOptions/WithSnapshot functor. But, again, I don't think many people outside of us ever used that feature of the API/low-level calling interface. I did deprecate the top-level (alias.go) WithOption and renamed it just Option, but that's not a breaking change due to continuing to keep the old WithOption type in place. So, I think (unless you say otherwise) this should be "safe" to cut as another 0.x release. :) |
|
Note IF we decide we bump the major version, we can and probably should revamp and conclude my ancient SRIOV PR. It's stuck because last time I worked on it I could not find a reasonnably high backward compatibility. |
Sound reasonnable. I didn't yet grasp this because I only had a cursor glance over your PR, which I did because I wanted to acknowledge this work and the review request. I will have a deep enough look ASAP and comment again. |
First and foremost. This is great work. Kudos! Now: I tend to agree the happy/common path should be relatively safe/straightforward, or so it seems reviewing the changes. And, truth is, the more I review the more I like this change, so I would really like to have it in. But I'm still worried about how breaking this breaking change is. I think stability is a good plus, and while this change has a lot of value for maintainers, I don't see a compelling case for users of If we can wither minimize the disruption (avoid it completely would be ideal ofc) AND clearly outline the value proposition, I would be much more comfortable and we can just go ahead. What I can do next is to try out a Some minor comments inside; I didn't go much further because, well, the PR is already pretty clean and I'd rather settle out the compat. conversation before to dive into details. |
|
@ffromani thank you, as always, for the thoughtful review and commentary. I very much appreciate your focus on stability and backwards compatibility! There are actually only two backwards-incompatible changes to the API/ABI included in this PR, both of which I believe are only applicable to a very small slice of users (perhaps only you and I ;) ):
All other changes to the main For example, if the user's code looked like this: package main
import "github.com/jaypipes/ghw"
func main() {
cpu, err := ghw.CPU()
if err != nil {
...
}
}nothing has changed about the call signatures for either the type-aliased Same for the following: package main
import "github.com/jaypipes/ghw"
func main() {
cpu, err := ghw.CPU(ghw.WithDisableTools())
if err != nil {
...
}
}or this: package main
import (
"github.com/jaypipes/ghw"
"github.com/jaypipes/ghw/pkg/option"
func main() {
cpu, err := ghw.CPU(option.WithChroot("/mnt/newroot/")
if err != nil {
...
}
}or even this: package main
import (
"github.com/jaypipes/ghw/pkg/cpu"
"github.com/jaypipes/ghw/pkg/option"
func main() {
info, err := cpu.New(option.WithChroot("/mnt/newroot/")
if err != nil {
...
}
}I'm fine creating a v1 release series git branch and redirecting this PR to that branch, but AFAICT, apart from the minor incompatible things related to pkg/memory.CachesForNode() and the snapshotting code, it's actually backwards-compatible... anyway, I'll leave it to you what you want to do. Happy to do the v1 branch thing but also happy to do a v0 dot release. |
|
Thanks @jaypipes! Again, your points make sense and the code looks good. I'm not by any means trying to diminuish this work! I can't think in any way I could have done better. Thing is, I've seen I guess I need to convince myself trying by doing the upgrade as I mentioned in the previous comment. I will attempt shortly. Thanks again for the patience and the explanation, and apologies for the delay in review. |
|
thinking about it, I have a proposal. What if we identify few (in the order of like 4-5) prominent importers of Something like "project XYZ compiles with no changes and passes all the tests" or "project ABC needs this patch but then compiles cleanly and passes all the tests" or (hopefully not) "project FOO compiles with no changes but fails tests 1, 4, 77". EDIT: obviously I volunteer to help implementing this proposal, getting in touch with these projects, submitting test PRs and socializing the changes. |
I like the proposal. What would be your short-list of importing projects? |
Since my proposal is related to the breaking changes assessment, but is not necessarily gating, I'm moving the conversation here: #421 and I sketched an initial very rough draft of a shortlist and the criterias to add/keep projects in the list Will try out #417 (comment) as soon as possible |
DNM Signed-off-by: Francesco Romani <[email protected]>
|
update: testing is going good! I need to run a few more tests (disclosure: going slower because vacation) but so far so good, looks like we're trending towards merge! |
|
update: I'm more and more inclined to merge. Other tasks heppened in between, but I plan to wrap up my tests and reach a decision "soon" |
cool, thanks for looking at this again @ffromani! :) I will rebase and fix up the conflicts this week. |
@ffromani sorry for the delay. now rebased and fixed all merge conflicts. :) |
jaypipes
force-pushed
the
context
branch
5 times, most recently
from
de72a15 to
e98098b
Compare
Gets rid of `pkg/context` entirely along with the `Do()/Setup()/Teardown()` context-handling stuff that was added to deal with snapshots. Moves to a simpler and more straight-forward system of passing zero or more Option functors that build an Options struct. Removes the `ghw-snapshot` tool and makes the `ghwc` tool understand ghw snapshots natively (just pass a `-s <snapshot_path>` CLI flag). Added a `ghwc snapshot` command that creates snapshots. Signed-off-by: Jay Pipes <[email protected]>
ffromani
left a comment
ffromani
left a comment
There was a problem hiding this comment.
Thanks @jaypipes for this work. This makes the codebase better and more modern, and my concern are largely addressed. My own test applications seem to be fine after a dep bump + recompile, as pointed out in the review.
In an ideal world, we should avoid API breakages and do only fully backward compatible changes. But at the same time, we should keep our codebase evolving and clean up even the darkest corners. After much consideration, I think this PR manages to strike a fine balance between all needs. I don't see obvious avoidable breakages (and if we are made aware, we will work out to address and prevent them), and the code is better than before.
So, while not ideal, this PR is good enough and IMO does much more good than else, so I'm fine to merge.
I think that (and I think we agree!) we need to release note carefully the changes here to make sure users clearly know what to expect. There are also other improvements we can add, like adding a generic pluggable logger (using the new slog stdlib package probably) and get rid of github.com/pkg/errors in favor of the stdlib package which we can do as followups;
If possible, I'd like us to discuss (and hopefully merges!) these changes before to cut another release so we minimize disruptions, but I'm flexible here.
Finally, a couple of minor, inline comments for future cleanups.
Thanks again Jay for this PR! keep up the good work!
| } | ||
| opts = append(opts, ghw.WithChroot(unpackDir)) | ||
| cmd.PersistentPostRunE = func(c *cobra.Command, args []string) error { | ||
| _ = os.RemoveAll(unpackDir) |
There was a problem hiding this comment.
Do we want to bubble up the error here? I tend to think yes but I'm not 100% sure
| if !debug { | ||
| return | ||
| } | ||
| fmt.Printf(msg, args...) |
There was a problem hiding this comment.
do we want to use stderr for tracing?
There was a problem hiding this comment.
Yeah, we should. I'll fix up in a future PR.
Continues the work from #417 in two import ways. First, this finalizes the move towards passing only a `context.Context` argument to the inner `InfoXXX.load()` methods instead of the old `pkg/option.Options` struct. Using a `context.Context` is more modern Go idiomatic and aligned with modern Go packages like `log/slog`. Each `InfoXXX.load()` method ensures a `context.Context` is created to store various options and context that is passed between modules in `ghw`. The function signature for the main module constructors like `pkg/cpu.CPU()` or `pkg/block.Block()` have all been changed from this: ```go func CPU(opt ...option.Option) (*Info, error) ``` to ```go func CPU(args ...any) (*Info, error) ``` which is a backwards compatible API change. We then examine each of the `args` arguments and set various keys on the `context.Context`. The `context.Context` is examined by other modules like `pkg/linuxpath` or `pkg/linuxdmi` instead of the prior `pkg/option.Options` struct. The second major part of this patch is the addition of log/output formatting to use `log/slog` and `log/slog.Handler` overrides to control the output of warning and debug log lines. The default log output in `ghw` only writes WARN-level messages to `stderr` in a simple `WARN: <msg>` log record format: ``` $ ghwc baseboard WARN: Unable to read board_serial: open /sys/class/dmi/id/board_serial: permission denied baseboard vendor=System76 version=thelio-mira-b4.1 product=Thelio Mira ``` You can control a number of log output options programmatically or by using environs variables. To change the log level `ghw` uses, set the `GHW_LOG_LEVEL` environs variable: ``` $ GHW_LOG_LEVEL=debug ghwc baseboard DEBUG: reading from "/sys/class/dmi/id/board_asset_tag" DEBUG: reading from "/sys/class/dmi/id/board_serial" WARN: Unable to read board_serial: open /sys/class/dmi/id/board_serial: permission denied DEBUG: reading from "/sys/class/dmi/id/board_vendor" DEBUG: reading from "/sys/class/dmi/id/board_version" DEBUG: reading from "/sys/class/dmi/id/board_name" baseboard vendor=System76 version=thelio-mira-b4.1 product=Thelio Mira ``` Changing `GHW_LOG_LEVEL` to `error` has the same effect of setting `GHW_DISABLE_WARNINGS`: ``` $ GHW_LOG_LEVEL=error ghwc baseboard baseboard vendor=System76 version=thelio-mira-b4.1 product=Thelio Mira ``` You can change the log level programmatically using the `WithLogLevel` modifier: ```go import ( "log/slog" "github.com/jaypipes/ghw" ) bb, err := ghw.Baseboard(ghw.WithLogLevel(slog.LevelDebug)) ``` To use the [logfmt][logfmt] standard log output format, set the `GHW_LOG_LOGFMT` envrions variable: ``` $ GHW_LOG_LOGFMT=1 ghwc baseboard time=2025-12-28T07:31:08.614-05:00 level=WARN msg="Unable to read board_serial: open /sys/class/dmi/id/board_serial: permission denied" baseboard vendor=System76 version=thelio-mira-b4.1 product=Thelio Mira ``` You can tell `ghw` to use `logfmt` standard output formatting using the `WithLogLogfmt` modifier: ```go import ( "log/slog" "github.com/jaypipes/ghw" ) bb, err := ghw.Baseboard(ghw.WithLogLogfmt()) ``` [logfmt]: https://www.cloudbees.com/blog/logfmt-a-log-format-thats-easy-to-read-and-write You can now programmatically override the logger that `ghw` uses with the `WithLogger` modifier. You pass in an instance of `slog.Logger`, like this example that shows how to use a simple logger with colored log output: ```go package main import ( "context" "encoding/json" "io" "log" "log/slog" "github.com/fatih/color" "github.com/jaypipes/ghw" ) type PrettyHandlerOptions struct { SlogOpts slog.HandlerOptions } type PrettyHandler struct { slog.Handler l *log.Logger } func (h *PrettyHandler) Handle(ctx context.Context, r slog.Record) error { level := r.Level.String() + ":" switch r.Level { case slog.LevelDebug: level = color.MagentaString(level) case slog.LevelInfo: level = color.BlueString(level) case slog.LevelWarn: level = color.YellowString(level) case slog.LevelError: level = color.RedString(level) } fields := make(map[string]interface{}, r.NumAttrs()) r.Attrs(func(a slog.Attr) bool { fields[a.Key] = a.Value.Any() return true }) b, err := json.MarshalIndent(fields, "", " ") if err != nil { return err } timeStr := r.Time.Format("[15:05:05.000]") msg := color.CyanString(r.Message) h.l.Println(timeStr, level, msg, color.WhiteString(string(b))) return nil } func NewPrettyHandler( out io.Writer, opts PrettyHandlerOptions, ) *PrettyHandler { h := &PrettyHandler{ Handler: slog.NewJSONHandler(out, &opts.SlogOpts), l: log.New(out, "", 0), } return h } func main() { opts := PrettyHandlerOptions{ SlogOpts: slog.HandlerOptions{ Level: slog.LevelDebug, }, } handler := NewPrettyHandler(os.Stdout, opts) logger := slog.New(handler) bb, err := ghw.Baseboard(ghw.WithLogger(logger)) if err != nil { logger.Error(err.String()) } fmt.Println(bb) } ``` Signed-off-by: Jay Pipes <[email protected]>
Continues the work from #417 in two import ways. First, this finalizes the move towards passing only a `context.Context` argument to the inner `InfoXXX.load()` methods instead of the old `pkg/option.Options` struct. Using a `context.Context` is more modern Go idiomatic and aligned with modern Go packages like `log/slog`. Each `InfoXXX.load()` method ensures a `context.Context` is created to store various options and context that is passed between modules in `ghw`. The function signature for the main module constructors like `pkg/cpu.CPU()` or `pkg/block.Block()` have all been changed from this: ```go func CPU(opt ...option.Option) (*Info, error) ``` to ```go func CPU(args ...any) (*Info, error) ``` which is a backwards compatible API change. We then examine each of the `args` arguments and set various keys on the `context.Context`. The `context.Context` is examined by other modules like `pkg/linuxpath` or `pkg/linuxdmi` instead of the prior `pkg/option.Options` struct. The second major part of this patch is the addition of log/output formatting to use `log/slog` and `log/slog.Handler` overrides to control the output of warning and debug log lines. The default log output in `ghw` only writes WARN-level messages to `stderr` in a simple `WARN: <msg>` log record format: ``` $ ghwc baseboard WARN: Unable to read board_serial: open /sys/class/dmi/id/board_serial: permission denied baseboard vendor=System76 version=thelio-mira-b4.1 product=Thelio Mira ``` You can control a number of log output options programmatically or by using environs variables. To change the log level `ghw` uses, set the `GHW_LOG_LEVEL` environs variable: ``` $ GHW_LOG_LEVEL=debug ghwc baseboard DEBUG: reading from "/sys/class/dmi/id/board_asset_tag" DEBUG: reading from "/sys/class/dmi/id/board_serial" WARN: Unable to read board_serial: open /sys/class/dmi/id/board_serial: permission denied DEBUG: reading from "/sys/class/dmi/id/board_vendor" DEBUG: reading from "/sys/class/dmi/id/board_version" DEBUG: reading from "/sys/class/dmi/id/board_name" baseboard vendor=System76 version=thelio-mira-b4.1 product=Thelio Mira ``` Changing `GHW_LOG_LEVEL` to `error` has the same effect of setting `GHW_DISABLE_WARNINGS`: ``` $ GHW_LOG_LEVEL=error ghwc baseboard baseboard vendor=System76 version=thelio-mira-b4.1 product=Thelio Mira ``` You can change the log level programmatically using the `WithLogLevel` modifier: ```go import ( "log/slog" "github.com/jaypipes/ghw" ) bb, err := ghw.Baseboard(ghw.WithLogLevel(slog.LevelDebug)) ``` To use the [logfmt][logfmt] standard log output format, set the `GHW_LOG_LOGFMT` envrions variable: ``` $ GHW_LOG_LOGFMT=1 ghwc baseboard time=2025-12-28T07:31:08.614-05:00 level=WARN msg="Unable to read board_serial: open /sys/class/dmi/id/board_serial: permission denied" baseboard vendor=System76 version=thelio-mira-b4.1 product=Thelio Mira ``` You can tell `ghw` to use `logfmt` standard output formatting using the `WithLogLogfmt` modifier: ```go import ( "log/slog" "github.com/jaypipes/ghw" ) bb, err := ghw.Baseboard(ghw.WithLogLogfmt()) ``` [logfmt]: https://www.cloudbees.com/blog/logfmt-a-log-format-thats-easy-to-read-and-write You can now programmatically override the logger that `ghw` uses with the `WithLogger` modifier. You pass in an instance of `slog.Logger`, like this example that shows how to use a simple logger with colored log output: ```go package main import ( "context" "encoding/json" "io" "log" "log/slog" "github.com/fatih/color" "github.com/jaypipes/ghw" ) type PrettyHandlerOptions struct { SlogOpts slog.HandlerOptions } type PrettyHandler struct { slog.Handler l *log.Logger } func (h *PrettyHandler) Handle(ctx context.Context, r slog.Record) error { level := r.Level.String() + ":" switch r.Level { case slog.LevelDebug: level = color.MagentaString(level) case slog.LevelInfo: level = color.BlueString(level) case slog.LevelWarn: level = color.YellowString(level) case slog.LevelError: level = color.RedString(level) } fields := make(map[string]interface{}, r.NumAttrs()) r.Attrs(func(a slog.Attr) bool { fields[a.Key] = a.Value.Any() return true }) b, err := json.MarshalIndent(fields, "", " ") if err != nil { return err } timeStr := r.Time.Format("[15:05:05.000]") msg := color.CyanString(r.Message) h.l.Println(timeStr, level, msg, color.WhiteString(string(b))) return nil } func NewPrettyHandler( out io.Writer, opts PrettyHandlerOptions, ) *PrettyHandler { h := &PrettyHandler{ Handler: slog.NewJSONHandler(out, &opts.SlogOpts), l: log.New(out, "", 0), } return h } func main() { opts := PrettyHandlerOptions{ SlogOpts: slog.HandlerOptions{ Level: slog.LevelDebug, }, } handler := NewPrettyHandler(os.Stdout, opts) logger := slog.New(handler) bb, err := ghw.Baseboard(ghw.WithLogger(logger)) if err != nil { logger.Error(err.String()) } fmt.Println(bb) } ``` Signed-off-by: Jay Pipes <[email protected]>
Continues the work from #417 in two import ways. First, this finalizes the move towards passing only a `context.Context` argument to the inner `InfoXXX.load()` methods instead of the old `pkg/option.Options` struct. Using a `context.Context` is more modern Go idiomatic and aligned with modern Go packages like `log/slog`. Each `InfoXXX.load()` method ensures a `context.Context` is created to store various options and context that is passed between modules in `ghw`. The function signature for the main module constructors like `pkg/cpu.CPU()` or `pkg/block.Block()` have all been changed from this: ```go func CPU(opt ...option.Option) (*Info, error) ``` to ```go func CPU(args ...any) (*Info, error) ``` which is a backwards compatible API change. We then examine each of the `args` arguments and set various keys on the `context.Context`. The `context.Context` is examined by other modules like `pkg/linuxpath` or `pkg/linuxdmi` instead of the prior `pkg/option.Options` struct. The second major part of this patch is the addition of log/output formatting to use `log/slog` and `log/slog.Handler` overrides to control the output of warning and debug log lines. The default log output in `ghw` only writes WARN-level messages to `stderr` in a simple `WARN: <msg>` log record format: ``` $ ghwc baseboard WARN: Unable to read board_serial: open /sys/class/dmi/id/board_serial: permission denied baseboard vendor=System76 version=thelio-mira-b4.1 product=Thelio Mira ``` You can control a number of log output options programmatically or by using environs variables. To change the log level `ghw` uses, set the `GHW_LOG_LEVEL` environs variable: ``` $ GHW_LOG_LEVEL=debug ghwc baseboard DEBUG: reading from "/sys/class/dmi/id/board_asset_tag" DEBUG: reading from "/sys/class/dmi/id/board_serial" WARN: Unable to read board_serial: open /sys/class/dmi/id/board_serial: permission denied DEBUG: reading from "/sys/class/dmi/id/board_vendor" DEBUG: reading from "/sys/class/dmi/id/board_version" DEBUG: reading from "/sys/class/dmi/id/board_name" baseboard vendor=System76 version=thelio-mira-b4.1 product=Thelio Mira ``` Changing `GHW_LOG_LEVEL` to `error` has the same effect of setting `GHW_DISABLE_WARNINGS`: ``` $ GHW_LOG_LEVEL=error ghwc baseboard baseboard vendor=System76 version=thelio-mira-b4.1 product=Thelio Mira ``` You can change the log level programmatically using the `WithLogLevel` modifier: ```go import ( "log/slog" "github.com/jaypipes/ghw" ) bb, err := ghw.Baseboard(ghw.WithLogLevel(slog.LevelDebug)) ``` To use the [logfmt][logfmt] standard log output format, set the `GHW_LOG_LOGFMT` envrions variable: ``` $ GHW_LOG_LOGFMT=1 ghwc baseboard time=2025-12-28T07:31:08.614-05:00 level=WARN msg="Unable to read board_serial: open /sys/class/dmi/id/board_serial: permission denied" baseboard vendor=System76 version=thelio-mira-b4.1 product=Thelio Mira ``` You can tell `ghw` to use `logfmt` standard output formatting using the `WithLogLogfmt` modifier: ```go import ( "log/slog" "github.com/jaypipes/ghw" ) bb, err := ghw.Baseboard(ghw.WithLogLogfmt()) ``` [logfmt]: https://www.cloudbees.com/blog/logfmt-a-log-format-thats-easy-to-read-and-write You can now programmatically override the logger that `ghw` uses with the `WithLogger` modifier. You pass in an instance of `slog.Logger`, like this example that shows how to use a simple logger with colored log output: ```go package main import ( "context" "encoding/json" "io" "log" "log/slog" "github.com/fatih/color" "github.com/jaypipes/ghw" ) type PrettyHandlerOptions struct { SlogOpts slog.HandlerOptions } type PrettyHandler struct { slog.Handler l *log.Logger } func (h *PrettyHandler) Handle(ctx context.Context, r slog.Record) error { level := r.Level.String() + ":" switch r.Level { case slog.LevelDebug: level = color.MagentaString(level) case slog.LevelInfo: level = color.BlueString(level) case slog.LevelWarn: level = color.YellowString(level) case slog.LevelError: level = color.RedString(level) } fields := make(map[string]interface{}, r.NumAttrs()) r.Attrs(func(a slog.Attr) bool { fields[a.Key] = a.Value.Any() return true }) b, err := json.MarshalIndent(fields, "", " ") if err != nil { return err } timeStr := r.Time.Format("[15:05:05.000]") msg := color.CyanString(r.Message) h.l.Println(timeStr, level, msg, color.WhiteString(string(b))) return nil } func NewPrettyHandler( out io.Writer, opts PrettyHandlerOptions, ) *PrettyHandler { h := &PrettyHandler{ Handler: slog.NewJSONHandler(out, &opts.SlogOpts), l: log.New(out, "", 0), } return h } func main() { opts := PrettyHandlerOptions{ SlogOpts: slog.HandlerOptions{ Level: slog.LevelDebug, }, } handler := NewPrettyHandler(os.Stdout, opts) logger := slog.New(handler) bb, err := ghw.Baseboard(ghw.WithLogger(logger)) if err != nil { logger.Error(err.String()) } fmt.Println(bb) } ``` Signed-off-by: Jay Pipes <[email protected]>
Gets rid of
pkg/contextentirely along with theDo()/Setup()/Teardown()context-handling stuff that was added to deal with snapshots. Moves to a simpler and more straight-forward system of passing zero or more Option functors that build an Options struct.Removes the
ghw-snapshottool and makes theghwctool understand ghw snapshots natively (just pass a-s <snapshot_path>CLI flag). Added aghwc snapshotcommand that creates snapshots.