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

Exclude doc/ from sdist #1408

Merged
merged 2 commits into from
Feb 19, 2023

Conversation

maresb
Copy link
Contributor

@maresb maresb commented Feb 5, 2023

It seems that the doc/ directory isn't doing anything but taking up space in the sdist. (Deleting reduces the .tar.gz sdist from 3.85MB to 1.34MB.)

Note: doc/.build/PLACEHOLDER is still included. It's 14 years old now, so perhaps it is unnecessary and can be deleted?

Closes #1397

Thank you for opening a PR!

Here are a few important guidelines and requirements to check before your PR can be merged:

  • There is an informative high-level description of the changes.
  • The description and/or commit message(s) references the relevant GitHub issue(s).
  • pre-commit is installed and set up.
  • The commit messages follow these guidelines.
  • The commits correspond to relevant logical changes, and there are no commits that fix changes introduced by other commits in the same branch/BR.
  • There are tests covering the changes introduced in the PR.

Don't worry, your PR doesn't need to be in perfect order to submit it. As development progresses and/or reviewers request changes, you can always rewrite the history of your feature/PR branches.

If your PR is an ongoing effort and you would like to involve us in the process, simply make it a draft PR.

@maresb
Copy link
Contributor Author

maresb commented Feb 5, 2023

In the spirit of changing no more than one thing at a time, I don't see any rush on merging this. We may want to wait until after the Hatch stuff is already released and shown to be stable.

@maresb
Copy link
Contributor Author

maresb commented Feb 5, 2023

Strange, this is failing with

OSError: License file does not exist: LICENSE.txt

This doesn't make any sense to me yet, and it builds for me locally. I wonder if this could be related in some way to #1409?

I'm out of time right now, so I'll have to come back to this if someone else doesn't get it first.

@brandonwillard brandonwillard added enhancement New feature or request setup and installation Relates to the setup and installation of Aesara labels Feb 6, 2023
@dgerlanc
Copy link
Contributor

Strange, this is failing with

OSError: License file does not exist: LICENSE.txt

This doesn't make any sense to me yet, and it builds for me locally. I wonder if this could be related in some way to #1409?

I'm out of time right now, so I'll have to come back to this if someone else doesn't get it first.

LICENSE.txt seems to be a symbolic link to docs/LICENSE.txt, which I'm guessing is the issue.

It seems that the doc/ directory isn't doing anything but taking
up space in the sdist. (Deleting reduces the .tar.gz sdist from 3.85MB
to 1.34MB.)

Note: doc/.build/PLACEHOLDER is still included. It's 14 years old now,
so perhaps it is unnecessary and can be deleted?

Closes aesara-devs#1397
Before: LICENSE.txt is a symlink to doc/LICENSE.txt
After: LICENSE.txt is no longer a symlink, doc/LICENSE.txt is deleted
@maresb maresb force-pushed the exclude-doc-dir-in-sdist branch from 07f7564 to 8b61148 Compare February 18, 2023 10:52
@maresb
Copy link
Contributor Author

maresb commented Feb 18, 2023

Thanks @dgerlanc for solving the mystery!

I think it's fairly standard these days to just have LICENSE.txt in the project root, and I suspect that doc/LICENSE.txt may be some legacy artifact. Let's see if deleting it breaks anything.

@brandonwillard brandonwillard merged commit 855a700 into aesara-devs:main Feb 19, 2023
@brandonwillard
Copy link
Member

Thanks as always, @maresb!

@maresb maresb deleted the exclude-doc-dir-in-sdist branch February 19, 2023 08:39
@maresb
Copy link
Contributor Author

maresb commented Feb 19, 2023

Thanks for the merge! Results are looking good:
image
image

@brandonwillard
Copy link
Member

Thanks for the merge! Results are looking good: image image

Wow, much better!

@maresb maresb mentioned this pull request Feb 23, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request setup and installation Relates to the setup and installation of Aesara
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove doc/ from sdist
3 participants