flit icon indicating copy to clipboard operation
flit copied to clipboard

convert git's path separators to match os.sep

Open mawillcockson opened this issue 4 years ago • 5 comments

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.

mawillcockson avatar Sep 30 '21 09:09 mawillcockson

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

takluyver avatar Nov 22 '21 10:11 takluyver

@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 avatar Nov 30 '21 22:11 mawillcockson

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:

https://github.com/pypa/flit/blob/9335bfbf23bf79b05b30c23e909c073a477e7f65/flit_core/flit_core/sdist.py#L159-L171

  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 avatar Dec 02 '21 14:12 mawillcockson

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

mawillcockson avatar Dec 02 '21 14:12 mawillcockson

I've rebased to include the namespace subpackage test.

mawillcockson avatar Dec 04 '21 16:12 mawillcockson