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

Make env_logger an optional dependency #1226

Merged
merged 5 commits into from
Nov 6, 2024

Conversation

wks
Copy link
Collaborator

@wks wks commented Nov 5, 2024

Now the built-in env_logger is guarded behind a Cargo feature "builtin_env_logger". It is a default feature, but can be disabled in Cargo.toml by setting dependencies.mmtk.default-features = false. In this way, VM bindings that want to implement its own logger can remove the env_logger crate from its dependencies.

Fixes: #744

Now the built-in env_logger is guarded behind a Cargo feature
"builtin_env_logger".  It is a default feature, but can be disabled
in Cargo.toml by setting `dependencies.mmtk.default-features = false`.
In this way, VM bindings that want to implement its own logger can
remove the env_logger crate from its dependencies.
@wks wks mentioned this pull request Nov 5, 2024
@wks wks added the PR-extended-testing Run extended tests for the pull request label Nov 6, 2024
@wks wks marked this pull request as ready for review November 6, 2024 04:12
@wks
Copy link
Collaborator Author

wks commented Nov 6, 2024

JikesRVM failed once with "Failing instruction starting at f7cbd615 wasn't in RVM address space", a known bug.

@wks wks requested a review from qinsoon November 6, 2024 04:36
Copy link
Member

@qinsoon qinsoon left a comment

Choose a reason for hiding this comment

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

LGTM

@wks wks added this pull request to the merge queue Nov 6, 2024
Merged via the queue into mmtk:master with commit 3575521 Nov 6, 2024
39 of 40 checks passed
@wks wks deleted the feature/optional-env-logger branch November 6, 2024 06:50
@k-sareen
Copy link
Collaborator

k-sareen commented Nov 6, 2024

I just have a quick question about this. Let's say in the future we add a new feature to the list of default features, is there a way to selectively disable builtin_env_logger? or will the binding implementer have to disable the entire default feature?

@wks
Copy link
Collaborator Author

wks commented Nov 6, 2024

I just have a quick question about this. Let's say in the future we add a new feature to the list of default features, is there a way to selectively disable builtin_env_logger? or will the binding implementer have to disable the entire default feature?

Unfortunately there is no way to selectively disable some default features for now. There is an issue (rust-lang/cargo#3126) for Cargo, and it is still open now. But the user can always copy the list of default features and explicitly enable some of them.

@k-sareen
Copy link
Collaborator

k-sareen commented Nov 6, 2024

In that case, maybe it is better to make it a negation? As in no_builtin_env_logger. Then we can remove it from the default list and since the feature is disabled by default, we get the built-in env_logger.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-extended-testing Run extended tests for the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make env_logger optional
3 participants