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

Consider adding support to setup.py path #4354

Closed
perone opened this issue Jul 10, 2018 · 20 comments
Closed

Consider adding support to setup.py path #4354

perone opened this issue Jul 10, 2018 · 20 comments
Assignees
Labels
Accepted Accepted issue on our roadmap Feature New feature
Milestone

Comments

@perone
Copy link

perone commented Jul 10, 2018

When using a project that is inside a different folder in a repository, one wants to install the project using the setup.py inside that folder instead of the root repository. Nowadays there is no way to specify the path for the setup.py file.

Is there any workaround for that ?

@stsewd
Copy link
Member

stsewd commented Jul 10, 2018

I don't think there is a current solution for this, but this probably will be implemented in the v2 on the configuration file (see #4298 (comment)).

@stsewd stsewd added the Feature New feature label Jul 10, 2018
@perone
Copy link
Author

perone commented Jul 10, 2018

Thank you @stsewd !

@agjohnson agjohnson added this to the YAML File Completion milestone Aug 3, 2018
@agjohnson
Copy link
Contributor

There are a few issues open on this. It probably makes sense, especially if we also have a way to set path for requirements file/etc. I think originally we were going to use the base configuration option for this. For a case where you have a foo/setup.py and docs/conf.py for example, this would not work, so I guess we need an explicit setup.py path?

@stsewd any opinions here?

@stsewd
Copy link
Member

stsewd commented Aug 3, 2018

@agjohnson I was thinking about this, what I understand, users want to specify their python project root, so rtd can install the project from there. If we use the base option (too general) it won't solve all cases. What I was thinking is having a python.path or python.installation_path or something like that, so this should only affect the python.install option. mkdocs and sphinx have its own paths relative to the configuration file

@agjohnson
Copy link
Contributor

yeah, i think it's probably possible to hit a case where the setup.py path is sibling to the docs/ path. Being explicit here is probably best.

Regarding schema, i think python.path implies altering PYTHON_PATH, and python.installation_path sounds a little like this too. I'd say we want something else for the namespace.

A couple other options:

  1. Allow multiple packages as a list. This mimics python.requirements a little, though we actually allow a list here. I say list as we might as well support multilple packages if we're supporting arbitrary paths:
python:
  packages:
    - foo/bar
    - bar/foo
  1. allow multiple packages as dicts (for meta data or configuration of build):
python:
  packages:
    - path: foo/bar
      name: bar
      some_setup_py_option_maybe: true
  1. this one is the most destructive and should have been part of our initial schema design probably, but put this all under python.install as a list of dicts, including requirements install pieces:
python:
  install:
    # these imply pip?
    - requirements: docs/requirements.txt
    - requirements: requirements.txt
    # this implies setup.py install?
    - package: foo/bar/
      some_setup_py_options_here_maybe: true
    # alternatively, maybe we're explicit about install type
    - type: setup.py
      path: foo/bar/
    - type: pip
      path: docs/requirements.txt

Any opinion on any of these options?

@stsewd
Copy link
Member

stsewd commented Aug 3, 2018

I haven't seen a project with multiple python packages, what would be the use case? Having a common repo for all packages with a single documentation point?

  1. We still need to know the requirements path for each package
  2. I like this one, not sure if we need name.
  3. A requirements file doesn't imply pip, as the user can install a setup.py project using pip too.
    I like the third option.

Maybe we can change the schema now... But not sure how many users need this now. Doesn't feel normal for me having multiple python projects in the same repo. Also, there is a real difference in using pip install . vs setup.py install? https://stackoverflow.com/questions/15724093/difference-between-python-setup-py-install-and-pip-install I think we can just use pip 🤔

Also, if there are multiple packages, each one may have a different requirements file or maybe None. I'd say we need something like we have now in the python key but for multiple packages. Maybe list of dicts for all the python key or just a single dict, that should be a little easy to implement at this point

@agjohnson
Copy link
Contributor

I haven't seen a project with multiple python packages, what would be the use case?

we haven't had the request a lot, and i don't have example projects, but we've had the request to support multiple paths. The repos are generally some form of a monorepo.

We still need to know the requirements path for each package

Example 1 above is only for installation of a package via setup.py given a path. It leaves requirements file config option where it is now.

A requirements file doesn't imply pip, as the user can install a setup.py project using pip too.

I meant that a requirements file implies pip install for the requirements file, as you can't install requirements via setup.py -- a requirements file is not a packaging concept, it's a deployment concept for non-packaged code.

The difference between pip install of a package and setup.py install of a package is that setup.py supports extra_requirements and other logic. A package should not have a requirements file. Requirements files are used when code isn't going to be packaged properly -- an application, for example. There shouldn't be a case where you need both setup.py and a requirements install step together.

This is going to get complex very quickly, so I'm not certain what exactly we need to support. However, if we wanted to support a python.install configuraiton, the following cases could be/are supported:

  • Package installation via setup.py
  • Requirements installation via requirements file
  • Package installation via pip for packages that use setup.cfg instead of setup.py
  • Package installation via package name (and qualifier?)
  • Package installation of a package via explicit path (for local packages)

Our potential schema might look like:

python:
  install:
    # Install requirements via pip
    - requirements: docs/requirements.txt
    # Install path `something/else` via pip
    - package: something/else
    # Install explicit package via pip
    - package: 'django<1.11'
    # Install root path as package via setup.py
    - package: .
      use_setup_py: true

@stsewd
Copy link
Member

stsewd commented Aug 30, 2018

Install explicit package via pip

I think we should leave this to the project dep, not to the rtd config file.

python:
  install:
    - requirements: path/to/req.txt
      package: .
      method: pip

Maybe we want to consider #3181 here too? method can be setup.py, pip and pipenv

@agjohnson
Copy link
Contributor

a package key is not just for dependencies, it's for installing from arbitrary paths in a repo:

python:
  install:
    - package: path/to/package/in/repo/

This is useful for the case where either the package is not top-level or the repo contains multiple packages.

@agjohnson agjohnson added Needed: design decision A core team decision is required Accepted Accepted issue on our roadmap labels Sep 27, 2018
@agjohnson
Copy link
Contributor

Marking this design decision as we have a little bit more work to do on a schema addition for this, but i think i like the python.install as a list option. Let's decide schema to best support these few cases and then can figure out how soon we should prioritize things.

@rtfd/core this is good issue to chime in on for design decision

@humitos
Copy link
Member

humitos commented Oct 1, 2018

I like Anthony's proposal from #4354 (comment) combined with the Santos' idea of method: [pip, setup_py, pipenv] (even pipenv is not supported yet, but thinking in the future) inside the python.install.package key.

The idea of python.install.requirements and python.install.package makes it really easy to follow and understand what's going to be installed and also make it extensible in different directions (we can add more specific options to package or requirements for example without introducing breaking changes)

@ericholscher
Copy link
Member

I don't have a strong feeling here, but agree a list of possible options seems the most flexible. It lets us expand it in the future without having to change the syntax, which is a nice win.

@agjohnson
Copy link
Contributor

inside the python.install.package key

Note: my suggestion is that python.install is a list, not a dict. My example above would be a complex example with multiple packages/requirement files.

I do have a couple issues with add method to the schema.

First, this seems odd as package is the wrong nomenclature:

python:
  install:
    - package: foo/
      method: pipenv

pipenv operates on paths, not on packages. I also don't think this does anything helpful, as this is going to be closely analogous to just calling pip -- that is, pipenv won't operate on a lock file and will be calling the system pip for install. Similarly, we shouldn't allow pipenv install on a package identifier like django>2

Similarly, I don't think it makes sense to install package identifiers with setup.py, like:

python:
  install:
    - package: django>2
      method: setup.py

So, I think:

  • we need to support pipenv install in a separate type
  • we need to limit setup.py to only install paths
  • we need to limit pipenv to only paths
  • our story around conda and pip is going to get confusing

All the possibilities, rolled into one config:

python:
  install:
    # Install package at path via pip
    - path: contrib/some/package
    # Install package at path via setup.py
    - path: contrib/some/other/package
      method: setup.py
      # I'd prefer:
      # use_pip: false
      # or:
      # use_setup: true
    # Install package via pip
    - package: django>2
    # Requirements via pip
    - requirements: requirements/docs.txt
    # Pipfile via pipenv
    - pipfile: .

@agjohnson agjohnson modified the milestones: YAML File Completion, 2.8 Oct 2, 2018
@stsewd
Copy link
Member

stsewd commented Oct 2, 2018

package: django>2

I don't think is a good idea to support that, users should specify that on their requirements, setup.py, or pipfile. We want builds to be reproducible and the rtd config file will only work on rtd (and we also will need to take care of command injections).

I think path makes more sense for me.

@agjohnson
Copy link
Contributor

Supporting package installation is not a strong requirement for this, but it's easy for us to implement if we're already making the above changes. Agreed it shouldn't be used often, but I'd rather offer it if it avoids users from having to maintain a separate requirements file just for docs dependencies.

I'm also considering non-python users when I say this. Having to explain dependencies and a requirements file is a lot for these users. It's much easier for these users to just define a package: django instead.

@stsewd
Copy link
Member

stsewd commented Oct 3, 2018

Just for confirmation of what we have:

If a user wants to install from a requirements.txt file:

python:
    install:
        - requirements: requirements.txt

If user wants to install from setup.py

python:
  install:
     - path: path/to/setup
       method: setuptools (instead of setup.py, so users don't misunderstand this value with the path?)

If user wants pip

python:
  install:
      - path: path/to/package
        method: pip

This lets the spec open to pipenv too.

Also, maybe support both a list and one element for install? Not sure if we are going to hit many cases of monorepos.

I prefer method or use, that way we have a smaller spec p:

@humitos
Copy link
Member

humitos commented Oct 4, 2018

@stsewd I think your previous comment is what we want.

Although, I'm marking some notes:

  • you missed the python.install.package option
  • I prefer python.install.method instead of python.install.use (use is too general to me)
  • python.install should be always a list in my opinion

@agjohnson
Copy link
Contributor

Agreed on all of that 👍 . method + path combination makes sense to me. We can start with the easier ones, and deal with pipenv support as a separate feature (though the schema is mostly defined now).

@humitos
Copy link
Member

humitos commented Nov 14, 2018

We all arrive to a proper solution and we have the pipfile schema already merged. I'm removing the Design decision label from this issue because we already known what we want to implement.

@humitos humitos removed the Needed: design decision A core team decision is required label Nov 14, 2018
@stsewd
Copy link
Member

stsewd commented Jan 22, 2019

This was already added in #4740.

@stsewd stsewd closed this as completed Jan 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Feature New feature
Projects
None yet
Development

No branches or pull requests

5 participants