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 foonathan-memory to fix memory allocation error on macOS #442

Merged
merged 1 commit into from
Sep 14, 2024

Conversation

stevalkr
Copy link
Contributor

@stevalkr stevalkr commented Aug 2, 2024

fixes [foonathan::memory] Allocator foonathan::memory::memory_pool error, mentioned in #419 (comment), see RoboStack/ros-humble#32 (comment)

Tested on macOS 15.0-beta, needs to be verified on other versions and platforms

@hegde-atri
Copy link

Seems to fix the issue and works perfectly fine on MacOS Sonama 14.5 👍🏼

@wentasah
Copy link
Contributor

wentasah commented Aug 3, 2024

Thanks @etherswangel for finding the fix. The file distros/humble/foonathan-memory-vendor/default.nix is automatically generated so don't try to modify it. It would be overwritten on the next superflore run. The change in the overrides.nix file is sufficient. However, I would extend the comment above it to make it clear why we are using newer version than what's release in nix. I suggest changing the comment to:

  # This is a newer version than the build system tries to download, but this
  # version doesn't try to run host platform binaries on the build platform
  # and fixes "Allocator foonathan::memory::memory_pool received invalid size"
  # error on MacOS.

@stevalkr
Copy link
Contributor Author

stevalkr commented Aug 4, 2024

Thanks, I've done what you said, please check.

@wentasah
Copy link
Contributor

wentasah commented Aug 4, 2024

Looks good. Could you also remove the first two commits? One way of doing that is using interactive rebase (e.g. git rebase -i HEAD~3) followed by git push -f.

@stevalkr
Copy link
Contributor Author

stevalkr commented Aug 4, 2024

Could you use Squash and merge? This way I won't have to force push my fork and there'll be only one new commit in this repo. If it's not allowed in this repo I'll squash and force push

@wentasah
Copy link
Contributor

wentasah commented Aug 8, 2024

I cannot merge here, but hopefully @lopsided98 can.

In general I don't see a problem with forcepushing in your forks. Github can show the diff between the old and new version so it's easy to verify that nothing has changed. On the other hand, squashing requires extra effort from the maintainer to edit the commit message. In this case it's mostly irrelevant, but with longer commit messages it would be nice to save the maintainer the extra work.

fixes [foonathan::memory] Allocator foonathan::memory::memory_pool error,
mentioned in lopsided98#419 (comment)
uecomment-2196472985, see https://github.com/RoboStack/ros-humble/issues/
32#issuecomment-2227591674
@stevalkr
Copy link
Contributor Author

Sorry for the delayed response, I’ve been swamped this week. I’ve pushed the changes, but I think most developers may want the —-force not to be with them. The commit message is just there, you don’t need to edit anything. Just two clicks and you’re all set

@wentasah
Copy link
Contributor

@stevalkr I guess the above merge was not intentional. I'm not sure how Github's "squash & merge" behaves in this case, but yet another force-push to revert the merge might help :-).

@stevalkr
Copy link
Contributor Author

@wentasah Sorry that was a mistake. I forgot this is a branch in PR and still haven’t got merged. Well it's been half a month since my last force-push so I doubt it's getting merged...

@lopsided98 would you take a look?

@lopsided98 lopsided98 merged commit a6f38ac into lopsided98:develop Sep 14, 2024
@stevalkr stevalkr mentioned this pull request Sep 15, 2024
6 tasks
@stevalkr stevalkr deleted the develop branch September 15, 2024 08:44
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.

4 participants