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 direct install from GitHub repo #202

Merged
merged 1 commit into from
Sep 5, 2024

Conversation

schuenke
Copy link
Collaborator

@schuenke schuenke commented Sep 4, 2024

With #195 we switched from setup.py to pyproject.toml, where we defined:

[tool.setuptools.packages.find]
where = ["pypulseq"]

This works fine when you install pypulseq from a local folder using pip install ., but not if you want to install it directly from GitHub using pip install git+https://github.com/imr-framework/pypulseq.

With the direct install from GitHub, no pypulseq package will be created, but only the modules in our pypulseq folder (like SAR, seq_examples, Sequence, ...) will be installed as packages.

For me, removing the 2 lines above solves the problem, which makes sense IMO, because this is meant to tell setuptools where to look for our package and not which folder IS our package. Setuptools detects the src-layout, where the pypulseq folder would be in a src folder (we should switch to this layout !!!) and the flat-layout (the one we are still using) automatically. This is why we don't have to specify where to look for our package at all.

@FrankZijlstra
Copy link
Collaborator

Not an expert on this, but looks fine. Surprising that the behaviour is different depending on the install options. Have you checked whether it affects packaging the pypi package?

@schuenke
Copy link
Collaborator Author

schuenke commented Sep 4, 2024

Not an expert on this, but looks fine. Surprising that the behaviour is different depending on the install options. Have you checked whether it affects packaging the pypi package?

I didn't test it on PyPi directly, but tried the build command locally and installed it from the generated wheel in a clean env. That worked. Therefore, I am pretty sure it will work with PyPi as well.

@btasdelen
Copy link
Collaborator

Yeah, sorry, apparently I misunderstood what it does, I thought it would add pypulseq as a package, too.

Would you like to convert to src hierarchy in this PR, or should we go ahead and merge?

@FrankZijlstra
Copy link
Collaborator

Moving everything to src might create big merge conflicts on the other PRs? I'd suggest to incorporate those first (and I have one more adding the sequence examples back to the sequence unit tests).

@schuenke
Copy link
Collaborator Author

schuenke commented Sep 5, 2024

I agree that we should merge this one as well as #190 and #200 first and then create a seperate PR for switching to the src-layout.

@schuenke schuenke merged commit 2f09cf6 into imr-framework:dev Sep 5, 2024
4 checks passed
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.

3 participants