Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
*: Remove default features from all crates #2918
*: Remove default features from all crates #2918
Changes from 22 commits
0f59322
9d59408
16a4692
ce0708a
a79d57c
148090f
8bf0ad7
e6b49a3
0a92268
c6f5090
a31f595
37c7c79
c5968d6
cbb66a5
9add85b
844adb5
4b8091d
26bf2b8
7f0a54c
0bf360b
fbee01e
32978fd
eaea056
58fa8ba
feb7c32
4f7010a
dbaf661
fc94c06
2450831
8af0768
fa6e027
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Say I want to run the
libp2p-core
unit tests locally, wouldn't it be nicer to only compile thelibp2p
features that I need? Is this use-case worth the additional effort?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a pain though to keep track of the features you need to activate :/
Plus, with all crates requesting different features, I actually think we have to compile things multiple times to satisfy each combination. If the convention is to enable
full
everywhere, we only need to compile every crate once to and the compiled artifact can be used for every crate's tests1.I think the convenience of just saying "full" is worth it. Locally you have caches of everything so you will most likely just pay in linker time and if you modify your code between test executions, you gotta relink the test binary anyway so I am not sure it will matter in practice.
Can we try it out and if it negatively affects anything, figure out how to make it better?
Footnotes
This is an unproven hypothesis. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for elaborating. Using
full
sounds good to me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed my mind. Esp. working on
libp2p-core
is quite painful now because every change there invalidateslibp2p-core
artifact which causes a re-compile of every other crate.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 mind proposing a patch @thomaseizinger?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think the best way to resolve this is to actually stop the circular dependency. This will allow for proper caching of the build artifacts. It will also solve the issue I am currently facing with
release-please
(#2902).