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

dcm2niix explicitly noted as a (PyPI) dependency and removed from being installed via apt-get etc #628

Merged
merged 4 commits into from
Feb 1, 2023
Merged

dcm2niix explicitly noted as a (PyPI) dependency and removed from being installed via apt-get etc #628

merged 4 commits into from
Feb 1, 2023

Conversation

TheChymera
Copy link
Contributor

@TheChymera TheChymera commented Jan 31, 2023 •
edited
Loading

Closes (part of) #627

@TheChymera TheChymera mentioned this pull request Jan 31, 2023
Closed
1 task
@yarikoptic
Copy link
Member

FTR -- it was only recently that dcm2niix got "Python package" on pypi contributed: https://github.com/rordenlab/dcm2niix/pull/605/files . What happens is that upon installation of the sources, it builds/installs the whl:

Building wheels for collected packages: dcm2niix
  Building wheel for dcm2niix (pyproject.toml) ... done
  Created wheel for dcm2niix: filename=dcm2niix-1.0.20220715-cp311-cp311-linux_x86_64.whl size=549462 sha256=336dea4b0cd2cef5754ed011566fb6f1d4792c9c056e6d595534ca9be6136609
  Stored in directory: /root/.cache/pip/wheels/36/8b/7d/26d61529a2df3157c9eecb9ea4ea4a45485fa28a1e9edae55f

I think it would only be great to become 'self pypi sufficient' so I do not see a reason to not do it. Do you see anything against it @satra @pvelasco and other @nipy/team-heudiconv ?

@codecov
Copy link

codecov bot commented Jan 31, 2023 •
edited
Loading

Codecov Report

Base: 81.30% // Head: 81.30% // No change to project coverage 👍

Coverage data is based on head (95d9d20) compared to base (a83a6bd).
Patch has no changes to coverable lines.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master     #628   +/-   ##
=======================================
  Coverage   81.30%   81.30%           
=======================================
  Files          41       41           
  Lines        3820     3820           
=======================================
  Hits         3106     3106           
  Misses        714      714
Impacted Files Coverage Δ
heudiconv/info.py 100.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@yarikoptic
RF: do not apt-get install or demand manual installation of dcm2niix …
7a0c8e4 
…in the docs
@yarikoptic
Do not bother with separate installation of dcm2niix in neurodocker
6c2cba8
@yarikoptic
[DATALAD RUNCMD] produce updated dockerfile
95d9d20 
=== Do not change lines below ===
{
 "chain": [
  "e179ee94e55f524620bea09c4d29fe56a7908fff"
 ],
 "cmd": "utils/gen-docker-image.sh",
 "exit": 0,
 "extra_inputs": [],
 "inputs": [],
 "outputs": [],
 "pwd": "."
}
^^^ Do not change lines above ^^^
@yarikoptic yarikoptic changed the title dcm2niix explicitly noted as a dependency dcm2niix explicitly noted as a (PyPI) dependency and removed from being installed via apt-get etc Feb 1, 2023
@yarikoptic yarikoptic added the internal Changes only affect the internal API label Feb 1, 2023
@yarikoptic
Copy link
Member

@TheChymera I have pushed a few more commits since we can not simplify some other aspects of installation/documentation. Please review and also check out more -- may be there is even more to it.

@TheChymera
Copy link
Contributor Author

@yarikoptic gave it another once-over, it looks like you caught all instances of dcm2niix being installed manually. I'd say it's good to go.

@yarikoptic
Copy link
Member

Let's give it a shot, thanks!

@yarikoptic yarikoptic merged commit 97d5f6b into nipy:master Feb 1, 2023
@TheChymera TheChymera deleted the dependencies branch February 1, 2023 21:11
@github-actions
Copy link

🚀 PR was released in v0.12.0 🚀

@yarikoptic yarikoptic mentioned this pull request Mar 14, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
internal Changes only affect the internal API released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@TheChymera @yarikoptic