Skip to content

Get sip.pyi in the Docker build #66

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

Merged
merged 4 commits into from
Sep 18, 2020

Conversation

altendky
Copy link
Collaborator

No description provided.

Dockerfile Outdated
# Install SIP including stubs
# TODO: Find way to build only stubs. This takes way too long
WORKDIR /upstream/
RUN pip install --no-deps --target build sip==${SIP_VERSION}
Copy link
Collaborator

@BryceBeagle BryceBeagle Sep 14, 2020

Choose a reason for hiding this comment

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

We might want to make this download style consistent with the other build layers.

I think when I wrote the rest of the Dockerfile, I downloaded directly from PyPI's CDN to avoid the python-pip install, but in hindsight that may have been overkill. I'm not opposed to making every layer just use pip.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, so now that I look the regular PyQt5 package provides its own .pyi files. Why are we building ourselves? (regardless of how we get the sdist)

Copy link
Collaborator

@BryceBeagle BryceBeagle Sep 15, 2020

Choose a reason for hiding this comment

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

They didn't used to be: #40
We may be able to avoid building at this point

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I vote we don't mix switching away from building into this right now. Maybe? Though I do like consistency. Turns out the .pyi are right in the sdist so I'll switch over to the download/extract/copy structure. We can revisit not-building separately.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is the update what you were looking for?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not familiar enough with the differences between find -name and find -wholename, but assuming you had a reason to go with the latter, lgtm

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I knew -name was wrong because I had the parent directory in the path but I ran it anyways and it told me to -wholename. At least the present sdist provides two ABIs so we need to pick one over the other.

@altendky altendky changed the title Get SIP in the Docker build Get sip.pyi in the Docker build Sep 15, 2020
@altendky
Copy link
Collaborator Author

@BryceBeagle, thanks for all your feedback and help with my work here.

@stlehmann stlehmann merged commit 58a4623 into python-qt-tools:upstream Sep 18, 2020
bluebird75 pushed a commit to bluebird75/PyQt5-stubs that referenced this pull request Aug 24, 2021
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