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.

Sign up for GitHub

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

Jump to bottom

crash converting QStringList to QSet #116

Closed
pieper opened this issue Aug 2, 2020 · 6 comments · Fixed by #117
Closed

crash converting QStringList to QSet #116

pieper opened this issue Aug 2, 2020 · 6 comments · Fixed by #117

Comments

@pieper
Copy link
Member

pieper commented Aug 2, 2020

Regression from #113 led to crashes as described in Slicer/Slicer#5078.
pieper added a commit that referenced this issue Aug 2, 2020
@pieper
BUG: fix crash calculating environment keys
be8ceaf 
Fixes #116

Each call to `ctkAppLauncherEnvironment::excludeReservedVariableNames` returns a new `QStringList` so making a QSet from the `begin()` of one and the `end()` of the other led to the crash in the `QHash`.

Solution is to make temporary variables for the `QStringList`s so that the set is made from matching iterators variables.

Also rearranged the code a bit for readability.
@pieper pieper mentioned this issue Aug 2, 2020
Merged
@jcfr jcfr closed this as completed in #117 Aug 2, 2020
@muratmaga
Copy link

This still crashes for me

@lassoan
Copy link
Member

lassoan commented Aug 3, 2020

How did you test it? You need a static build of Qt, which requires commercial license.

@jcfr
Copy link
Member

jcfr commented Aug 3, 2020

You need a static build of Qt, which requires commercial license.

Since the launcher is a standalone executable, we build Qt statically our-self and comply with the license requirements.

@jamesobutler
Copy link
Contributor

Note the CTKAppLauncher git hash has not been updated https://github.com/Slicer/Slicer/blob/1a8a34060e491e659c7edd0dc2a7171ca12278ad/SuperBuild/External_CTKAppLauncherLib.cmake#L29

@jcfr
Copy link
Member

jcfr commented Aug 3, 2020

To move forward:

@muratmaga
Copy link

muratmaga commented Aug 3, 2020 •
edited

How did you test it? You need a static build of Qt, which requires commercial license.

I just downloaded the latest preview and installed dcm2niix and try to import one of @cpinter tiny patient datasets via DICOMBrowser after dcm2niix install.

@sjh26 sjh26 mentioned this issue Sep 21, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

BUG: fix crash calculating environment keys commontk/AppLauncher
5 participants
@pieper @jcfr @lassoan @jamesobutler @muratmaga