Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Added #subdirectory tag specify a relative subdirectory inside a repo… #526

Closed
wants to merge 58 commits into
from

Conversation

Projects
None yet
Contributor

niedbalski commented May 9, 2012

Is possible to use a relative path inside the repo to reference a subdirectory the #subdirectory tag is used in the edit mode.

pip install -e https://github.com/niedbalski/test.git#egg=test&subdirectory=subpackage_module

Tests passed.

Contributor

pnasrat commented May 13, 2012

Related to Issue #209

I'd like to see some design discussion with others @carljm and @jezdez before taking the approach of adding another url fragment to handle this, particularly as we're considering other ways of passing build information such as --install-options. However thanks for providing a tested CL for this.

@pnasrat pnasrat and 1 other commented on an outdated diff May 13, 2012

tests/test_vcs_backends.py
@@ -15,6 +15,16 @@ def test_install_editable_from_git_with_https():
result.assert_installed('pip-test-package', with_files=['.git'])
+def test_install_editable_with_subdirectory():
+ """
+ Test installing a package from a repo subdirectory
+ """
+ reset_env()
+ result = run_pip('install', '-e',
+ '%s#egg=pip-test-package#subdirectory=piptestsubpackage' %
+ local_checkout('git+https://github.com/niedbalski/pip-test-package.git'),
+ expect_error=True)
+
@pnasrat

pnasrat May 13, 2012

Contributor

You are not making any assertions in this test. You probably want an result.assert_installed line here. Also I'd rather your test package be named for the test, eg pip-test-subdir-package else it may confuse someone in the future.

@niedbalski

niedbalski May 14, 2012

Contributor

Oops. Added assertion to the test :)

@pnasrat pnasrat and 1 other commented on an outdated diff May 13, 2012

pip/req.py
@@ -1303,6 +1315,15 @@ def parse_requirements(filename, finder=None, comes_from=None, options=None):
req = InstallRequirement.from_line(line, comes_from)
yield req
+def has_subdirectory(editable_req):
+ """
+ Search for subdirectory parameter on editable URL
+ Returns False if not found or the subdirectory name if success
+ """
+ match = re.search(r'.*(?:#|#.*?&)subdirectory=([^&]*)', editable_req)
@pnasrat

pnasrat May 13, 2012

Contributor

This feels to me like the logic is perhaps in the wrong place, and we should be handling this in the class not as a module level method.

I'm also not sure if this is not RFC breaking http://tools.ietf.org/html/rfc1808.html

 <scheme>://<net_loc>/<path>;<params>?<query>#<fragment>
@niedbalski

niedbalski May 14, 2012

Contributor

Also not sure about this. Reading about multiple url fragments, appears that this should be implementation-dependent.

I think that this works pretty well for adding optional installation parameters.

Contributor

niedbalski commented May 14, 2012

Added the commented changes.

Contributor

niedbalski commented May 22, 2012

Any ideas about this?

Contributor

carljm commented May 22, 2012

I think the url fragment is right for this, as it's really specifying the location of the package to be installed, not a build option for the installation. This was already discussed a while back on the mailing list.

I think, however, that the syntax when providing multiple bits of info in the fragment should be #subdirectory=foo&egg=bar not #subdirectory=foo#egg=bar.

Contributor

pnasrat commented May 23, 2012

That seems to make sense and is valid and according to http://en.wikipedia.org/wiki/Fragment_identifier others use that approach

   pchar         = unreserved / pct-encoded / sub-delims / ":" / "@"
   query         = *( pchar / "/" / "?" )
   fragment      = *( pchar / "/" / "?" )
   pct-encoded   = "%" HEXDIG HEXDIG
   unreserved    = ALPHA / DIGIT / "-" / "." / "_" / "~"
   reserved      = gen-delims / sub-delims
   gen-delims    = ":" / "/" / "?" / "#" / "[" / "]" / "@"
   sub-delims    = "!" / "$" / "&" / "'" / "(" / ")"
                 / "*" / "+" / "," / ";" / "="
@niedbalski niedbalski Editable URL query string to options, all the options can use a proce…
…ss_option ( i.e. process_egg ) callback to validate the value, refactor and cleanup. tests passed OK
39f3e35
Contributor

niedbalski commented May 23, 2012

Please review the diff . according to your request all the query string options are passed in the format #option=foo&egg=bar , the options are passed to the Installable instance as editable_options function argument, also if you want to pre-process a specific parameter ( i.e. egg ) just provide a process_xxx method.

For the subdirectory option this has been added:

+            if 'subdirectory' in self.editable_options:
+                setup_py_path = os.path.join(os.path.dirname(self.setup_py),
+                                                            self.editable_options['subdirectory'], \
+                                                            os.path.basename(self.setup_py))

Also i did some small code cleanup.

This pull request fails (merged 39f3e35 into 6369d53).

This pull request fails (merged b681064 into 6369d53).

This pull request fails (merged dac305d into 6369d53).

This pull request fails (merged 126b1ba into e0db44e).

This pull request fails (merged e720e7f into e0db44e).

This pull request fails (merged fc1abb4 into e0db44e).

This pull request fails (merged ccc38d4 into e0db44e).

Contributor

pnasrat commented May 23, 2012

CI errors are known bad #503, but means everything else is passing with your chage.

@carljm carljm commented on an outdated diff May 23, 2012

pip/req.py
@@ -208,10 +214,20 @@ def run_egg_info(self, force_root_egg_info=False):
logger.indent += 2
try:
script = self._run_setup_py
- script = script.replace('__SETUP_PY__', repr(self.setup_py))
+
+ if 'subdirectory' in self.editable_options:
@carljm

carljm May 23, 2012

Contributor

This logic should simply be embedded within the self.setup_py property; self.setup_py should always point to the setup.py.

@carljm carljm and 1 other commented on an outdated diff May 23, 2012

pip/req.py
@@ -1303,48 +1319,103 @@ def parse_requirements(filename, finder=None, comes_from=None, options=None):
req = InstallRequirement.from_line(line, comes_from)
yield req
+def process_subdirectory(value):
+ """
+ Search for subdirectory parameter on editable URL
+ Returns False if not found or the subdirectory name if success
+ """
+ return value
@carljm

carljm May 23, 2012

Contributor

What is the purpose of this function if it simply returns the value it is passed? The behavior does not seem to match the docstring at all.

@niedbalski

niedbalski May 23, 2012

Contributor

Removed from code this is just an example.

@carljm carljm and 1 other commented on an outdated diff May 23, 2012

pip/req.py
@@ -1303,48 +1319,103 @@ def parse_requirements(filename, finder=None, comes_from=None, options=None):
req = InstallRequirement.from_line(line, comes_from)
yield req
+def process_subdirectory(value):
+ """
+ Search for subdirectory parameter on editable URL
+ Returns False if not found or the subdirectory name if success
+ """
+ return value
+
+def process_egg(req):
@carljm

carljm May 23, 2012

Contributor

this should have a more descriptive name - perhaps egg_basename

@niedbalski

niedbalski May 23, 2012

Contributor

Sorry @carljm i think that the idea es to have a map between method process_name == query string attribute name for code maintenance and extension.

What do you think?

Contributor

niedbalski commented May 23, 2012

Added changes to include @carljm observations.

@carljm carljm and 1 other commented on an outdated diff May 23, 2012

tests/test_vcs_backends.py
@@ -14,6 +14,15 @@ def test_install_editable_from_git_with_https():
expect_error=True)
result.assert_installed('pip-test-package', with_files=['.git'])
+def test_install_editable_with_subdirectory():
+ """
+ Test installing a package from a repo subdirectory
+ """
+ reset_env()
+ result = run_pip('install', '-e',
+ '%s#egg=pip-test-subdir-package#subdirectory=piptestsubpackage' %
+ local_checkout('git+https://github.com/niedbalski/pip-test-subdir-package.git'), expect_error=True)
@carljm

carljm May 23, 2012

Contributor

local_checkout and reliance on network resources is problematic - we have it a lot, but I don't want to introduce any more of it, especially since this test is not necessarily tied to git.

Instead, can we put the test package directly in tests/packages and install it editable using the filesystem path?

@niedbalski

niedbalski May 23, 2012

Contributor

Not sure about this, for now this works. I will create a new test and i will add this to packages.

@carljm

carljm May 23, 2012

Contributor

Yes, local_checkout does work, it just introduces nondeterministic behavior in the tests (because sometimes they actually get the repo from the network, and sometimes they use a locally-cached version), which makes it hard to get reliably stable test results. So I'm trying to avoid use of it in new tests in favor of having everything local whenever possible.

@niedbalski

niedbalski May 28, 2012

Contributor

OK, added as git+file , and tested installation. Test file added to packages.

This pull request passes (merged 785a6ab into e0db44e).

@carljm carljm and 1 other commented on an outdated diff May 23, 2012

pip/req.py
+ This method generates a dictionary of the query string
+ parameters contained in a given editable URL.
+ if the process_attribute function exists then is called
+ and the returned value is used.
+ """
+ regexp = re.compile(r"[\?#&](?P<name>[^&=]+)=(?P<value>[^&=]+)")
+ matched = regexp.findall(req)
+
+ if matched:
+ ret = dict()
+ for option in matched:
+ (name, value) = option
+ if name in ret:
+ raise Exception("%s option already defined" % name)
+ try:
+ value = globals()['process_%s' % name](value)
@carljm

carljm May 23, 2012

Contributor

No, this is too much magic. Remove this entire process_* thing and leave options["egg"] as exactly the value found in the url fragment. The stripping of the version part of the egg should happen where options["egg"] is pulled out and used as the requirement name - it's the requirement name that should not have the version part, it's fine if options["egg"] continues to have it. Does that make sense?

@niedbalski

niedbalski May 23, 2012

Contributor

Ok, i moved the process_egg method to strip_postfix. BTW i think that the process* method call is still needed for more complex processing logic on the query string parameters ( feature ).

Thanks you.

@carljm

carljm May 23, 2012

Contributor

Hi Jorge,

I'm afraid I don't understand what the process_* function call is needed for. It's not needed for egg, and it's not needed for subdirectory, and those are the only two url-fragment options that pip understands. I don't think we need all this code (especially not code which accesses globals(), generally a bad sign) just because hypothetically we might someday add more url fragment options that might use it.

@niedbalski

niedbalski May 23, 2012

Contributor

I understand your point. I removed the process_x call, but i think that if you are providing dynamic query string parameters , in the future you will end with a lot of if-else code for parameter processing vs. a simple function call to a developer-specific function.

This pull request passes (merged b6c7e80 into e0db44e).

This pull request fails (merged 6334276 into e0db44e).

Contributor

niedbalski commented May 28, 2012

Please review the diff , maybe now can be merged :)

Contributor

niedbalski commented Jun 1, 2012

Hi guys, did you reviewed this patch ?

Contributor

pnasrat commented Jun 2, 2012

I'm in the process of merging a large change atm for Issue #511 we've done 2 of the 5 part plan https://gist.github.com/2822510

Currently your pull request isn't able to be merged automatically. I'd like to clear the 3 further patches from #511 then I'll ping this issue to make sure you've got it on top of current develop and fixed any merge conflicts, then review and merge.

Contributor

niedbalski commented Jun 3, 2012

Perfect. Ping me back when you are ok with the #511

Contributor

niedbalski commented Jun 12, 2012

Hello Paul,

I merged pending conflicts from upstream.

This pull request fails (merged d9abc60 into ae867db).

Contributor

pnasrat commented Jun 13, 2012

Lots of failures in the areas you are changing - can you check the tests pass locally. See the Travis build fail link for details.

Contributor

pnasrat commented Jul 1, 2012

Ping on checking failures.

orutherfurd and others added some commits Jun 7, 2012

@orutherfurd @niedbalski orutherfurd + niedbalski fix compatability w/3.3 & 2.7.4 & bazaar (#552) 7b9d788
@orutherfurd @niedbalski orutherfurd + niedbalski non_hierarchical was dropped at the same time as uses_fragment (#552) 11f1fe7
@niedbalski lepture + niedbalski fix on git repo. if a repo contains submodules, checkout submodules f701a9d
@niedbalski lepture + niedbalski fix #533 . quite and recursive on submodule checkout 1686520
@qwcode @niedbalski qwcode + niedbalski reset_env option to add a patch to sitecustomize.py ca97a11
@qwcode @niedbalski qwcode + niedbalski not expecting errors in these tests. let them pass errors. 997f441
@qwcode @niedbalski qwcode + niedbalski make sure sitecustomize.py doesn't grow in the FastTestPipEnvironment f571f19
@fin @niedbalski fin + niedbalski test submodule support 2390c95
@fin @niedbalski fin + niedbalski fix python2.5 syntax error fd4cd1e
@fin @niedbalski fin + niedbalski remove "git submodule foreach" call for backwards compatibility; expe…
…ct errors in pip install because old versions of git write senseless things to stderr
58b950b
@carljm @niedbalski carljm + niedbalski Update AUTHORS and changelog for git submodules. 5ade518
@niedbalski unknown + niedbalski Updated basecommand.py such that writing to the log file doesn't fail…
…. If

permission is denied for writing in the specified log file the message
will be written to a temporary one.
027e0d8
@carljm @niedbalski carljm + niedbalski Update changelog. dc62187
@msabramo @niedbalski msabramo + niedbalski Make magic use pkg_resources.iter_entry_points instead of relying on
egg_info.iter_entry_points, which might not be there if someone
redefines egg_info (like the setup.py for pyobjc-core does)

Fixes GH-11
e931c6a
@qwcode @niedbalski qwcode + niedbalski don't --user install in --system-site-packages virtualenvs with conflict c1847ec
@msabramo @niedbalski msabramo + niedbalski Add HackedEggInfo test for the the fix for GH-11. 4da91bc
@qwcode @niedbalski qwcode + niedbalski raise InstallationError when UninstallPathSet has no paths 9218ece
@qwcode @niedbalski qwcode + niedbalski use nose.tools.assert_raises 6827d04
@pnasrat @niedbalski pnasrat + niedbalski Fix failing bzr vcs test c318660
@pnasrat @niedbalski pnasrat + niedbalski Fix failng windows tests.
path_to_url uses os.path.normcase that breaks this test on windows.

Mocked out to use posix paths.
b7d4341
@pnasrat @niedbalski pnasrat + niedbalski Prevent assertion error from scripttest on windows.
We're seeing failure from the helpers due to CRLF messages when we use
git.
9bb12b5
@pnasrat @niedbalski pnasrat + niedbalski Skip submodule testing on windows.
Blocking release, the tests are very path orientated this needs fixup so
added TODO and skip for now.
eaa891b
@pnasrat @niedbalski pnasrat + niedbalski Fix failing windows test.
I spent a good while trying to debug this. Got working avoiding
local_checkout but want to fix correctly. However this will unblock
release.
328600d
@pnasrat @niedbalski pnasrat + niedbalski Fix missing import in test. 2247a2f
@pnasrat @niedbalski pnasrat + niedbalski Fix incorrect mocking. 46e3217
@qwcode @niedbalski qwcode + niedbalski generate error in test same way as pip.req 9add29c
@niedbalski niedbalski [pip/tests] added subpackage to _create_test_package method, fixed te…
…sts for support subdirectory
5c143aa
@niedbalski niedbalski [pip/req.py] modified options and editable_options validation 726fa13

This pull request passes (merged 726fa13 into ae867db).

Contributor

niedbalski commented Jul 3, 2012

Hello Paul,

Everything has been merged and tests are passing , merge ? Good Luck!,

Contributor

pnasrat commented Jul 5, 2012

I'm away for a wedding this weekend - not forgotten but I may not be able to get to for a few days

Contributor

niedbalski commented Jul 24, 2012

Hello @pnasrat did you reviewed this patchset?

Contributor

pnasrat commented Jul 24, 2012

Sorry just been busy - it's not automergable and it looks like it has a lot of unrelated stuff in it from when you synced and merged with develop. Can you ensure the differences are just your patch, once it's clean to apply I'll take another look.

Contributor

niedbalski commented Jul 24, 2012

OK @pnasrat i removed the conflicts and updated from upstream per your request, i hope you can merge/and close the pr. regards.

This pull request fails (merged d2552f0 into c6789f6).

Contributor

pnasrat commented Jul 24, 2012

This still looks a little off on eyeballing -I'm going to have to manually merge, probably not got time until the weekend.

Contributor

pnasrat commented Jul 30, 2012

I looked at merging this and the number of commits Jul 3 from others concerns me https://github.com/pypa/pip/pull/526/commits

As does changes that have been made by others in your diff such as tests/test_user_site.py

I tried to interactively rebase ontop of development but there were a lot of conflicts then. I don't want to screw up our git history inadvertently by merging this.

You may need to build a clean branch on top of develop - add your own changes and ensure it's clean and tests run then file a new PR.

@noirbizarre noirbizarre referenced this pull request in mapnik/mapnik Oct 22, 2012

Closed

Python setuptools/distutils packaging #1455

edevil commented Nov 5, 2012

This seems a simple and useful change. Is anyone looking at this?

Flimm commented Feb 14, 2013

+1

Contributor

niedbalski commented Mar 18, 2013

Cleaning this code and re-creating a new PR.

@niedbalski niedbalski closed this Mar 18, 2013

Julian commented May 26, 2013

@niedbalski I don't see a new PR. Still working on this?

+1

Contributor

rutsky commented Jul 3, 2013

+1

I'm interested in this feature too.

yulis commented Jul 24, 2013

+1
Really need this

Contributor

niedbalski commented Jul 24, 2013

Hello Guys.

I created another PR #1082 for this feature with the #subdirectory=foo editable option.

Regards.

+1 for this feature
really want to put pydmtx-wrappers setup into my pip-requiremets.txt

Is this also implemented for mecurial now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment