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

fix: fix crash in dataset list command on macos #576

Merged
merged 6 commits into from
Oct 25, 2021

Conversation

ivan-aksamentov
Copy link
Member

@ivan-aksamentov ivan-aksamentov commented Oct 25, 2021

Resolves #575

In order to build on Mac, I had to upgrade conan to the latest version due to conan-io/conan-center-index#5197. And the latest version seems to be only available on pip, not brew.

In f91247b (#576) I also adjusted the ci config to produce the binaries on non-release branch to be able to test the fix manually. This commit will be reverted before merge.

@vercel
Copy link

vercel bot commented Oct 25, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/nextstrain/nextclade/EkiMD5iwComBYq6HKfrm2reKU4Aw
✅ Preview: https://nextclade-git-fix-dataset-list-crash-nextstrain.vercel.app

@@ -394,7 +394,6 @@ namespace Nextclade {

auto numTags = std::to_string(datasetRef.versions.size());
fmt::format_to(buf, " Specific versions ({:}):\n", datasetRef.versions.size());
fmt::format_to(buf, " ------------------" + std::string{"-", numTags.size() + 3} + "\n\n");
Copy link
Member Author

@ivan-aksamentov ivan-aksamentov Oct 25, 2021

Choose a reason for hiding this comment

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

This is the "fix".

This temporary string seems to be crashing on MacOS in Release mode only. fmt considers it an invalid string, but I cannot figure out why.

Removing this line for now, it is non-essential. But there might be some deeper problem elsewhere, like memory corruption or otherwise undefined behavior. It's C++ we need to be alert (or rewrite Nextclade in Rust).

Additionally I updated the fmt library to incorporate latest fixes, in case something was wrong there.

This version introduces bugfixes and related api changes
Copy link
Member

@corneliusroemer corneliusroemer left a comment

Choose a reason for hiding this comment

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

LGTM, tested and it no longer throws an error on my Mac

@corneliusroemer corneliusroemer merged commit 7b5cbce into master Oct 25, 2021
@corneliusroemer corneliusroemer deleted the fix/dataset-list-crash branch October 25, 2021 19:32
@ivan-aksamentov
Copy link
Member Author

@corneliusroemer A bit too early, I need to revert some of the temporary changes I mentioned above first.

@corneliusroemer
Copy link
Member

Oops, sorry, I won't merge again...

@ivan-aksamentov
Copy link
Member Author

It's okay, I submitted these as a follow up in #578

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.

On mac nextclade dataset list --name=sars-cov-2 throws error [ERROR] Nextclade: invalid format string
2 participants