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

Update dependencies #586

Merged
merged 2 commits into from Jan 31, 2022
Merged

Conversation

alexandrasandulescu
Copy link

Hi. This PR bumps smoltcp dependencies version to latest according to crates.io.
The env_logger update brings an API change, the parse Builder method was replaced with parse_filters.

Apart from parse_filters, I noticed that the original code parsed RUST_LOG after setting the log level to trace and parsing the filters. My understanding is that this overwrites the builder setup with what RUST_LOG contains.
My change parses the default environment first with from_default_env and then applies the filter changes.

If the desired behaviour is the original one (overwrite setup_logging_with_clock filters with the ones defined by RUST_LOG) let me know.

Tnx :)

@alexandrasandulescu
Copy link
Author

One test fails but it looks unrelated to the changes. I cannot rerun the jobs in the test workflow.

@crawford
Copy link
Contributor

What’s the benefit of raising the minimum required version for these dependencies? E.g. is there a specific behavior that is being corrected?

As for the parsing of RUST_LOG, I think it’s generally desirable to allow environment variables to override defaults. This is how rustc and cargo handle the variable, for example.

@alexandrasandulescu
Copy link
Author

What’s the benefit of raising the minimum required version for these dependencies? E.g. is there a specific behavior that is being corrected?

The current dependencies version specification do not allow updates for env_logger, rand and url (according to Dependencies specification).
I use smoltcp in a setting where only certain dependencies versions are available (eg. env_logger > 0.9).

As for the parsing of RUST_LOG, I think it’s generally desirable to allow environment variables to override defaults. This is how rustc and cargo handle the variable, for example.

env_log defines "RUST_LOG" as default environment, which led me to the (wrong) assumption that it should be parsed first. Thank you for clarifying this issue.

@adamgreig
Copy link
Contributor

The current dependencies version specification do not allow updates for env_logger, rand and url (according to Dependencies specification).

That makes sense for those three dev-dependencies; is there any need to (or benefit from) updating the four dependencies byteorder/log/libc/bitflags?

* allow logging environment overwrite by RUST_LOG
@alexandrasandulescu
Copy link
Author

That makes sense for those three dev-dependencies; is there any need to (or benefit from) updating the four dependencies byteorder/log/libc/bitflags?

No. I added another commit based on your feedback. The PR now updates only env_logger, rand and url. Moreover, the RUST_LOG env overrides the default logging setup.

@crawford
Copy link
Contributor

The current dependencies version specification do not allow updates for env_logger, rand and url

I’m confused by this. My understanding of the documentation is that env_logger and rand could be updated, since the major version doesn’t change. Am I mistaken?

@adamgreig
Copy link
Contributor

adamgreig commented Jan 29, 2022

In Cargo, versions 0.x.y are effectively treated with x as the major and y as the minor version, so ^0.4.5 will use 0.4.6, but not 0.5.0. Once the first 1.0 version is published, ^1.2.3 would use 1.3.4 but not 2.0.

So, in this case, env_logger ^0.5 wouldn't use 0.9, and rand ^0.3 wouldn't use 0.8.

https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#caret-requirements

This compatibility convention is different from SemVer in the way it treats versions before 1.0.0. While SemVer says there is no compatibility before 1.0.0, Cargo considers 0.x.y to be compatible with 0.x.z, where y ≥ z and x > 0.

@crawford
Copy link
Contributor

Ah, I forgot about that caveat. Thanks for explaining.

Copy link
Member

@Dirbaio Dirbaio left a comment

Choose a reason for hiding this comment

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

This is nice, thanks for the PR! :)

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 31, 2022

Build succeeded:

@bors bors bot merged commit 61639d5 into smoltcp-rs:master Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants