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

convert git's path separators to match os.sep #447

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

mawillcockson
Copy link

git returns file paths separated by / regardless of the platform.

After flit.build.main() is called, starting from this line:

The current tests fake listing the files in the directory, but do not list the dist/module1-0.1.tar.gz that flit creates as untracked.

I changed the mock git script to behave a little more similarly to the git commands that would be called.

The simpler fix would be to have the mock git script return dist/module1-0.1.tar.gz when run with --deleted, but this way each test can specify the tracked files, and the files produced by flit are dynamically discovered.

I can understand if this makes the tests too flaky.

flit/sdist.py Outdated Show resolved Hide resolved
tests/test_build.py Outdated Show resolved Hide resolved
tests/test_build.py Outdated Show resolved Hide resolved
tests/test_build.py Outdated Show resolved Hide resolved
@takluyver
Copy link
Member

@mawillcockson do you think you'll get back to this, or would you like me to take it over?

@mawillcockson
Copy link
Author

@mawillcockson do you think you'll get back to this, or would you like me to take it over?

Yes, I can finish it, sorry for the delay.

I went down a rabbit hole trying to find out if os.fsdecode() is necessary, or if git will return UTF-8 regardless of the filesystem encoding, and os.fsdecode() may not decode non-ASCII text correctly:

https://github.com/takluyver/flit/blob/353f920b7a1a95c28ba1234d275324c351d6f4a3/flit/vcs/git.py#L6-L9

I wasn't able to come to a conclusion, so I'm leaving it as-is with a note, in case someone finds a bug with it.

@mawillcockson
Copy link
Author

I don't think we have any tests where this will actually find 'untracked' files at the moment, so this extra complexity isn't actually helping us test anything. Are you planning to add such a test?

Apologies: I abandoned this PR for so long, I forgot most of the context.

None of the tests currently find the untracked files, because the original mock script was not designed to print any.

For example, if git were real in the test_build_main test, this section of flit would be run:

def build(self, target_dir, gen_setup_py=True):
os.makedirs(str(target_dir), exist_ok=True)
target = target_dir / '{}-{}.tar.gz'.format(
self.metadata.name, self.metadata.version
)
source_date_epoch = os.environ.get('SOURCE_DATE_EPOCH', '')
mtime = int(source_date_epoch) if source_date_epoch else None
gz = GzipFile(str(target), mode='wb', mtime=mtime)
tf = tarfile.TarFile(str(target), mode='w', fileobj=gz,
format=tarfile.PAX_FORMAT)
try:
files_to_add = self.apply_includes_excludes(self.select_files())

  1. Line 160 creates the dist directory
  2. Lines 166-168 make a module1-0.1.tar.gz file in that directory
  3. Line 171 eventually runs both:
    1. git ls-files --deleted --others --exclude-standard -z which will return dist/module1-0.1.tar.gz with a forward slash, regardless of the platform
    2. git ls-files --recurse-submodules -z which returns the files that are part of module1_toml: EG-README.rst, module1.py, and pyproject.toml

The original script simulated only the git ls-files --recurse-submodules -z call.

The more complicated rewrite of the script I added and then subsequently removed did both.

In light of #445 adding tests with real git, I agree that my script was too complex.

I think adding a helper function could minimize the added complexity?

I also added a test that explicitly runs the build step twice. I think it's possible for the build steps outlined above to change, and running it twice ensures the second build will interact with the previous build's artifacts (e.g. the dist/ folder).

@mawillcockson
Copy link
Author

mawillcockson commented Dec 2, 2021

I have confirmed that, without the addition of os.path.normpath(), the tests fail on Windows, and pass on Linux.

@mawillcockson
Copy link
Author

I've rebased to include the namespace subpackage test.

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.

2 participants