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

cargo: fix jemalloc dependencies #27074

Merged

Conversation

nrainer-materialize
Copy link
Contributor

@nrainer-materialize nrainer-materialize commented May 14, 2024

@nrainer-materialize
Copy link
Contributor Author

@benesch, is this still up to date with recent changes regarding jemalloc?

Specifics for jemalloc

We enable jemalloc by default on all non-mac platforms, unless --no-default-features is enabled. In this case, jemalloc should be disabled on all platforms. To quickly determine the validity of changes, check that only the file x86_64-unknown-linux-gnu-default mentions the tikv-jemallocator dependency, and all others don't.

Copy link
Member

@benesch benesch left a comment

Choose a reason for hiding this comment

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

We need to go the other way with this! As described here, those dependencies are not unused but load bearing for selecting the correct memory allocator by default: #27067

I'll push up a revision to this PR that marks them as ignored in the unused dependency list.

@benesch
Copy link
Member

benesch commented May 14, 2024

@benesch, is this still up to date with recent changes regarding jemalloc?

Ah, not quite. Pushed up a revision that updates the text here appropriately.

@benesch benesch enabled auto-merge May 14, 2024 13:17
Copy link
Member

@antiguru antiguru left a comment

Choose a reason for hiding this comment

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

LGTM after the most recent changes!

@nrainer-materialize nrainer-materialize changed the title cargo: remove unused dependencies cargo: fix jemalloc dependencies May 14, 2024
@benesch benesch force-pushed the cargo/remove-unused-deps branch 2 times, most recently from 95db186 to 7e2ebbe Compare May 14, 2024 16:52
Comment on lines 73 to 82
| tail -r \
| awk -F: 'BEGIN {f=-1} /^([0-9]+):'"$crate"'/{print;f=$0-1} f==$1{print;f=f-1}' \
| tac)
| tail -r)
# Only print crate and dependency chain if there is a dependency.
Copy link
Member

Choose a reason for hiding this comment

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

Is tail -r really a thing?

tail -r
tail: invalid option -- 'r'
Try 'tail --help' for more information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably only on macOS? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Sigh. Shell scripts. Fixed.

@benesch benesch merged commit ea31bb2 into MaterializeInc:main May 14, 2024
76 checks passed
@nrainer-materialize nrainer-materialize deleted the cargo/remove-unused-deps branch May 15, 2024 07:17
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

Successfully merging this pull request may close these issues.

None yet

4 participants