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

Stabilize and document __log. #95

Closed
dpc opened this issue Aug 29, 2016 · 17 comments
Closed

Stabilize and document __log. #95

dpc opened this issue Aug 29, 2016 · 17 comments

Comments

@dpc
Copy link

dpc commented Aug 29, 2016

Sometimes needs arise where one wants to deliver a custom logging statement to log crate. __log is exactly that function, but then anyone using it is risking breaking changes.

It seems to me once log API is considered stable, __log should be documented and stabilized as well.

@sfackler
Copy link
Member

sfackler commented Aug 29, 2016

__log and its associated struct LogLocation are explicitly designed to be unstable and undocumented forever, since we are otherwise forever prevented from e.g. taking advantage of a future function!() macro to extend the location metadata.

@dpc
Copy link
Author

dpc commented Aug 29, 2016

The content of LogLocation can be extended without breaking anything, no?

@sfackler
Copy link
Member

I'm a bit confused - why do you need to use __log if you're not assembling your own LogLocation?

@dpc
Copy link
Author

dpc commented Aug 29, 2016

RIght. Hmm... LogLocation could be assembled eg. by a macro, that in the future would be changed to fill the missing field with a defaults ... But I guess that's too much complication for no good reason.

Will there be any other addition? I mean - one could just add function : &'static str field right away and for now fill it with "".

@sfackler
Copy link
Member

Who knows? function was just something I pulled from the top of my head.

@dpc
Copy link
Author

dpc commented Aug 30, 2016

Can't then log just bump version to 2.0 or something? :)

@sfackler
Copy link
Member

Bumping the major version of log is pretty traumatic ecosystem-wise. Until everyone upgrades you'll be losing log messages from anything that still talks to the old version. It is not a thing I want to do every time we want to add some new metadata to log events.

@sfackler
Copy link
Member

FYI, the only reason __log is public at all is that macros aren't hygenic with respect to privacy. It is the definition of an implementation detail.

@dpc
Copy link
Author

dpc commented Aug 30, 2016

I have a need to log things with custom meta information using LogLocation: https://github.com/dpc/slog-rs/blob/8ef65309610125e1ba024ae1d4f2eb4e2a0b58cb/crates/stdlog/lib.rs#L220

And it seems to me it's reasonable requirement, as "faking" line/file/module might be useful in some cases, when people don't want real information for whatever reason.

What if __log and LogLocation were stabilized, and any time anything change there, __logv2 and LogLocationv2 are created, while keeping __log and LogLocation as #[deprecated] backward compatibility forwarding things to __logv2? Macros itself will just use v2 versions.

@alexcrichton
Copy link
Member

The functionality here seems pretty reasonable to me, integrating one log system into another. Stabilizing exactly __log, however, will likely never happen. (or at least I would prefer such)

I wonder if we could perhaps explore a more first class API here? Something like:

struct LogBuilder {
    // opaque
}

impl LogBuilder {
    fn new() -> LogBuilder { ... }

    fn line(&mut self, line: u32) -> &mut LogBuilder { ... }
    fn module(&mut self, module: &str) -> &mut LogBuilder { ... }
    // other builder methods ...

    fn emit(&mut self) { ... }
}

This way slog and other libraries could programmatically build log structures and the log crate also has the ability to add more fields over time if need be. The _log method would also stay as-is today, the implementation would likely just change.

Thoughts?

@sfackler
Copy link
Member

We'd need to change that API a bit since line, module, etc are already required, unless we'd have emit panic which seems unfortunate.

@alexcrichton
Copy link
Member

Perhaps yeah, although we could also just add arguments to new

@dpc
Copy link
Author

dpc commented Aug 31, 2016

Anything wrong with versioned __log and LogLocation?

@alexcrichton
Copy link
Member

I personally much prefer extensible APIs to "versioned APIs". That way everyone always has the same ergonomic experience and it's not a huge diff to use new features.

@dpc
Copy link
Author

dpc commented Aug 31, 2016

Yes, normally me to. But in that case the API is kind of a backdoor anyway, and except few people noone can see it anyway.

My concerns are:

  • builder boilerplate could not always get completely optimized away or will get in a way of some optimizations
  • could there be any non-trivial changes to the logging record structure there would lead to troubes for the Builder having to support both new and old format?

If the above are not an issue, then Builder is clearly nicer.

@alexcrichton
Copy link
Member

Oh I wasn't worried about perf here because I figured that once you hit this point you've concluded "yes a log should happen" and whatever happens there will completely dwarf some code to assemble the message.

It's true yeah though that if a builder were stabilized we'd have to be very confident that it would be sufficient for an incredibly long time. I'm pretty confident in this, though, as the builders in the standard library have worked out very well so far.

@sfackler
Copy link
Member

Fixed by #211.

Techcable added a commit to Techcable/stdlog that referenced this issue Mar 23, 2022
Previously this API was required to work around rust-lang/log#95
Thanks to the inclusion of a `RecordBuilder` (PR rust-lang/log#211)
it is no longer required.

This fixes slog-rs/slog#309
This supersedes PR slog-rs#20

I have added a comment about supporting the log/kv_unstable feature in
the future.
Not supporting this feature doesn't break backwards compat, because the
old usage of the private API didn't support it either. Therefore, this
new code doesn't break or remove anything that was already existing.
However,from a feature-completeness standpoint it would be nice to have
this in the future.
Techcable added a commit to Techcable/stdlog that referenced this issue Mar 23, 2022
Previously this API was required to work around rust-lang/log#95

Thanks to the inclusion of a `RecordBuilder` (PR rust-lang/log#211)
it is no longer required.

This fixes slog-rs/slog#309
This supersedes PR slog-rs#20

I have added a comment about supporting the log/kv_unstable feature in the future.
Not supporting this feature doesn't break backwards compat, because the
old usage of the private API didn't support it either. Therefore, this
new code doesn't break or remove anything that was already existing.
However,from a feature-completeness standpoint it would be nice to have
this in the future. (see comment in code)
EFanZh pushed a commit to EFanZh/log that referenced this issue Jul 23, 2023
This from feedback in rust-lang#19:

> wrt. bin-dir and bin-path, this appears to be a typo / should all be called bin-dir

This is only a readme fix afaict, I changed all occurences of `bin-path` in there to `bin-dir`.

> wrt. format, those are actually two (unfortunately named) different concepts, the first
refers to the archive format (eg. .tgz), the second to the binary format (which needs a .exe
appended for windows).

This introduces two new substitutions:
- `binary-ext` is the old "`format` in `bin-dir`"
- `archive-format` is the old "`format` in `pkg-url`"

Contents are unchanged: `binary-ext` includes the dot, `archive-format` doesn't. That
makes it easy to upgrade and also personally I slightly prefer it that way.

The old contextual `format` is still available, "soft deprecated": it will be accepted silently
so everything will work, but all documentation will use the new syntax. In the future we
could move to a "hard deprecated" model where installing a package that uses `format`
will warn the user / tell them to report that to the maintainer. I don't think we'll ever really
be able to remove it but that should be good enough.

A cool new feature is that `binary-ext` is now usable in `pkg-url`, which will be useful for raw binary downloads:

```toml
pkg_url = "{ repo }/releases/download/v{ version }/{ name }-v{ version }-{ target }{ binary-ext }"
```

I've also added a bunch of tests to GhCrateMeta around the templating for `pkg-url`.
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

No branches or pull requests

3 participants