Skip to content

Comments

Create source distributions on macOS during build#384

Merged
ellbosch merged 8 commits intomasterfrom
ellbosch/sdist
Dec 12, 2019
Merged

Create source distributions on macOS during build#384
ellbosch merged 8 commits intomasterfrom
ellbosch/sdist

Conversation

@ellbosch
Copy link
Collaborator

@ellbosch ellbosch commented Dec 6, 2019

Closes #322.

This PR adds support for creating gztar and zip source distributions. The sdist call is made during the build phase, which is also when wheel files are created.

Furthermore, I added a check to only call sdist if the platform is macOS. This is because unlike wheel files, it is not required to generate source distributions on every platform we wish to support*, since we can generate both the gztar and zip from macOS.

*I need to test this hypothesis :)

@chlafreniere
Copy link
Collaborator

Please make sure the checks pass 😄

if sys.platform == 'darwin':
utility.exec_command('%s setup.py check -r -s sdist --formats=gztar,zip' % PYTHON,
utility.ROOT_DIR, continue_on_error=False)

Copy link
Collaborator

Choose a reason for hiding this comment

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

please test the entire product E2E with these changes

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe by uploading to pypi test 😄

continue_on_error=False)

# generate sdist--only run on Mac to eliminate redundant copies when published to Azure
if sys.platform == 'darwin':
Copy link
Collaborator

Choose a reason for hiding this comment

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

please document that mac wasn't chosen for a reason (you just needed to choose one platform) to avoid future confusion

@ellbosch
Copy link
Collaborator Author

@chlafreniere: a few changes made since our meeting yesterday:

  • zip file option removed (in favor of targz) since only one sdist can be supported
  • minor changes to variable names to make things clearer
  • version bump

release.py Outdated
# Only authorized users with credentials will be able to upload this package.
# Credentials will be stored in a .pypirc file.
for wheel in mssqlcli_wheel_dir:
for f in mssqlcli_dist_dir:
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is "f" here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Stands for file. I wanted to disambiguate this from wheels since we have an sdist now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we use "file" here instead, to avoid ambiguity?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, unfortunately, since file is a builtin. I'm open to a better name here--maybe file_dist?

Copy link
Collaborator

@corivera corivera left a comment

Choose a reason for hiding this comment

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

Approved pending other comments.

@ellbosch ellbosch merged commit 31062c5 into master Dec 12, 2019
@ellbosch ellbosch deleted the ellbosch/sdist branch December 12, 2019 17:43
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.

Create an sdist distribution

3 participants