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

GH-83417: Allow venv to add a .gitignore file to environments via a new scm_ignore_file parameter #108125

Merged
merged 12 commits into from
Sep 15, 2023

Conversation

brettcannon
Copy link
Member

@brettcannon brettcannon commented Aug 18, 2023

Off by default via code but on by default via the CLI, the .gitignore file contains * which causes the entire directory to be ignored.


📚 Documentation preview 📚: https://cpython-previews--108125.org.readthedocs.build/

Off by default via code but on by default via the CLI, the `.gitignore` file contains `*` which causes the entire directory to be ignored.
@brettcannon brettcannon linked an issue Aug 18, 2023 that may be closed by this pull request
Doc/library/venv.rst Outdated Show resolved Hide resolved
Doc/library/venv.rst Outdated Show resolved Hide resolved
Doc/library/venv.rst Outdated Show resolved Hide resolved
Doc/library/venv.rst Outdated Show resolved Hide resolved
Lib/test/test_venv.py Outdated Show resolved Hide resolved
brettcannon and others added 2 commits August 21, 2023 15:11
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
@ambv
Copy link
Contributor

ambv commented Aug 24, 2023

Realistically no other version control systems are popular enough to warrant this feature. I like the discoverability of the current wording of the new arguments.

The barrier of entry for a future version control system to become popular enough to warrant addition here is pretty high. Therefore, I don't think we need to bikeshed this anymore. Let's just add it so we can test it in practice.

@brettcannon
Copy link
Member Author

Looking at the 👍 on the comment from @ambv , people so far seem to prefer keeping the gitignore argument. Anyone prefer the approach suggested by @vsajip ?

Doc/library/venv.rst Outdated Show resolved Hide resolved
Lib/venv/__init__.py Outdated Show resolved Hide resolved
@brettcannon
Copy link
Member Author

I updated the PR using an scm_ignore_file parameter that takes as a string the name of the SCM to create an ignore file for. It dispatches to create_{scm_ignore_file}_ignore_file() for the file creation. I also made the CLI option be --without-scm-ignore-file. That should give @gaborbernat the flexibility he was after for virtualenv and anyone who customizing virtual environment creation via subclassing EnvBuilder.

I made the parameter take a single SCM name as no one realistically stores their source code in multiple version control systems. And since the ignore file prevents you from checking in the virtual environment, there's no need to commit a multitude of ignore files when developing a package.

I stuck with an opt-out flag since there's only git support right now and you can't make the CLI use a custom EnvBuilder class. If there's a desire to add a CLI option in the future we can add that in. But honestly, while I was writing the tests for a --with-scm-ignore-file, I realized the effort was not worth it for the CLI right now, especially when even pyvenv.cfg wouldn't record the option as it's the default semantics and defaults are left out. So YAGNI, avoiding pre-mature API optimization, and better things to do with my time on a long weekend made me drop --with-scm-ignore-file.

Hopefully this works for everyone. The API becomes flexible while the CLI stays simple which I think covers everyone's key concerns. If @ambv wants to drop his approval due to this change I would understand.

@brettcannon brettcannon changed the title GH-83417: Allow venv add a .gitignore file to environments GH-83417: Allow venv to add a .gitignore file to environments via a new scm_ignore_file parameter Sep 4, 2023
@gaborbernat
Copy link
Contributor

gaborbernat commented Sep 4, 2023

I made the parameter take a single SCM name as no one realistically stores their source code in multiple version control systems

Well this might be true, but imagine the case when I work at the company that for all internal projects uses SCM x. That being said, company still operates in a world where most upstream projects will use git. Shouldn't in this case a Hypothetically internal plugin by default disable version control on both SCM types? Or do you imagine that the caller will manually every time explicitly set the type of SCM the project in the current folder uses? The alternative would be too allow having multiple SCM targets in parallel. The additional SCM files for a different type of SCM would be automatically ignored by the matching SCMs configuration file, so there's no real downside in generating multiple such configurations, one per each SCM. Now granted even with the current design one could set this SCM to x,git as a string marker to generate both SCM configuration files but feels a bit odd to me.

@hugovk
Copy link
Member

hugovk commented Sep 4, 2023

I made the parameter take a single SCM name as no one realistically stores their source code in multiple version control systems.

It's not common, but there are repos that are stored in one SCM and mirrored to another, sometimes mirroring to GitHub to use the CI.

It's hard to find one on request, but for example https://sourceforge.net/p/docutils/code/HEAD/tree/ is SVN and https://repo.or.cz/docutils.git is Git, although this only contains a .gitignore.

(https://github.com/python/devinabox and https://github.com/pypa/distlib contain both a .hgignore and .gitgnore. https://git.launchpad.net/beautifulsoup/tree/ has both .bzrignore and .gitignore. I'm unsure if these are relics or still both in use.)

But I don't know if it's worth the extra complexity for these rare cases.

@ambv
Copy link
Contributor

ambv commented Sep 4, 2023

If @ambv wants to drop his approval due to this change I would understand.

I don't mind. I don't have any fundamental reason against supporting many SCMs, I just think it's unnecessary to spend so much brain power on it.

@ofek
Copy link
Contributor

ofek commented Sep 4, 2023

FWIW Hatchling supports multiple version control systems for default exclusion patterns: https://github.com/pypa/hatch/blob/hatchling-v1.18.0/backend/src/hatchling/builders/config.py#L798-L838

It's annoying to have in the code base but the risk of not supporting hypothetical users who would then open feature requests anyway is worse than the maintenance cost.

@brettcannon
Copy link
Member Author

Well this might be true, but imagine the case when I work at the company that for all internal projects uses SCM x. That being said, company still operates in a world where most upstream projects will use git. Shouldn't in this case a Hypothetically internal plugin by default disable version control on both SCM types?

It's not common, but there are repos that are stored in one SCM and mirrored to another, sometimes mirroring to GitHub to use the CI.

But the file isn't getting committed to the repository just as the virtual environment isn't. So you only have to care about the version control system where you're creating the virtual environment which won't be multiple SCMs but just the one being used locally.

Or do you imagine that the caller will manually every time explicitly set the type of SCM the project in the current folder uses?

The caller can determine what it wants to specify however it chooses. Also note we are talking about companies that are big enough to be running two SCMs; they have the staff to support this. 😉

The question is whether we should optimize the API for all possibilities because we only expect people with non-standard SCMs to set anything, or do we optimize for the the 99% of the world who only operate under a single SCM?

@brettcannon
Copy link
Member Author

Also, if we are talking about multiple SCMs, then that means you're already subclassing which means you can override __init__() and create() however you want.

If it bothers people that much we could add a create_ignore_file() method which handles the dispatch to the appropriate create_*_ignore_file() method(s) and that lets you do whatever you want.

@gaborbernat
Copy link
Contributor

I don't think supporting 1+ SCMs is any more complicated than exactly 1 in this case, but the current ApI is a bit less user-friendly but I digress, I made my point.

Lib/test/test_venv.py Outdated Show resolved Hide resolved
@brettcannon
Copy link
Member Author

I'll give @vsajip the week in case he wants to get in a review, otherwise I will plan to merge this on Friday.

@brettcannon brettcannon enabled auto-merge (squash) September 15, 2023 22:14
@brettcannon brettcannon enabled auto-merge (squash) September 15, 2023 22:14
@brettcannon brettcannon merged commit e218e50 into python:main Sep 15, 2023
17 of 18 checks passed
@brettcannon brettcannon deleted the issue-83417-venv-gitignore branch September 15, 2023 22:38
@brettcannon
Copy link
Member Author

Thanks everyone!

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot s390x RHEL7 LTO + PGO 3.x has failed when building commit e218e50.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/244/builds/5454) and take a look at the build logs.
  4. Check if the failure is related to this commit (e218e50) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/244/builds/5454

Failed tests:

  • test.test_asyncio.test_subprocess

Failed subtests:

  • test_subprocess_consistent_callbacks - test.test_asyncio.test_subprocess.SubprocessThreadedWatcherTests.test_subprocess_consistent_callbacks

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.x.edelsohn-rhel-z.lto-pgo/build/Lib/test/test_asyncio/test_subprocess.py", line 788, in test_subprocess_consistent_callbacks
    self.loop.run_until_complete(main())
  File "/home/dje/cpython-buildarea/3.x.edelsohn-rhel-z.lto-pgo/build/Lib/asyncio/base_events.py", line 664, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "/home/dje/cpython-buildarea/3.x.edelsohn-rhel-z.lto-pgo/build/Lib/test/test_asyncio/test_subprocess.py", line 780, in main
    self.assertEqual(events, [
AssertionError: Lists differ: [('pi[29 chars]t'), 'pipe_connection_lost', ('pipe_data_recei[57 chars]ted'] != [('pi[29 chars]t'), ('pipe_data_received', 2, b'stderr'), 'pi[57 chars]ted']

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot PPC64LE RHEL7 3.x has failed when building commit e218e50.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/446/builds/3899) and take a look at the build logs.
  4. Check if the failure is related to this commit (e218e50) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/446/builds/3899

Failed tests:

  • test.test_multiprocessing_fork.test_misc

Failed subtests:

  • test_nested_startmethod - test.test_multiprocessing_fork.test_misc.TestStartMethod.test_nested_startmethod

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le/build/Lib/test/_test_multiprocessing.py", line 5475, in test_nested_startmethod
    self.assertEqual(results, [2, 1])
AssertionError: Lists differ: [1, 2] != [2, 1]

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot PPC64LE RHEL7 LTO + PGO 3.x has failed when building commit e218e50.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/43/builds/4239) and take a look at the build logs.
  4. Check if the failure is related to this commit (e218e50) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/43/builds/4239

Failed tests:

  • test.test_asyncio.test_events

Failed subtests:

  • test_create_connection_local_addr_nomatch_family - test.test_asyncio.test_events.PollEventLoopTests.test_create_connection_local_addr_nomatch_family

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto-pgo/build/Lib/test/test_asyncio/test_events.py", line 715, in test_create_connection_local_addr_nomatch_family
    with self.assertRaises(OSError):
AssertionError: OSError not raised

csm10495 pushed a commit to csm10495/cpython that referenced this pull request Sep 28, 2023
…ts via a new `scm_ignore_file` parameter (pythonGH-108125)

This feature is off by default via code but on by default via the CLI. The `.gitignore` file contains `*` which causes the entire directory to be ignored.

Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
hroncok added a commit to hroncok/virtualenv that referenced this pull request Nov 29, 2023
…venv

Since Python 3.13, the .gitignore file is created by venv:
python/cpython#108125

Fixes pypa#2670
hroncok added a commit to hroncok/virtualenv that referenced this pull request Nov 29, 2023
…venv

Since Python 3.13, the .gitignore file is created by venv:
python/cpython#108125

Fixes pypa#2670
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.

[venv] Adding a .gitignore file to virtual environments
10 participants