Giter VIP home page Giter VIP logo

Comments (4)

thundergolfer avatar thundergolfer commented on July 23, 2024 1

Thanks for this, I see exactly what you're saying now.

I wonder if ruamel_yaml_clib is mistakenly listing yaml as one of its namespace packages, or if we're incorrectly following the PEP spec 🤔 . If both ruamel_yaml_clib and ruamel_yaml declare that they have yaml then that'd be a mistake on their end I think.

from rules_python_external.

thundergolfer avatar thundergolfer commented on July 23, 2024

rules_python_external creates and empty directory ....

Can you post the ls output from these package's Bazel external repositories if you've got it? would be good to get more detail.

As an aside, ruamel.yaml has some really complicated namespace packaging logic 😕.

from rules_python_external.

ironpeak avatar ironpeak commented on July 23, 2024 
cd bazel-bin/example.runfiles/__main__/external && find .

Output

.
./bazel_tools
./bazel_tools/tools
./bazel_tools/tools/python
./bazel_tools/tools/python/py3wrapper.sh
./__init__.py
./jiko_runtime_requirements
./jiko_runtime_requirements/pypi__ruamel_yaml_clib
./jiko_runtime_requirements/pypi__ruamel_yaml_clib/__init__.py
./jiko_runtime_requirements/pypi__ruamel_yaml_clib/_ruamel_yaml.cpython-38-darwin.so
./jiko_runtime_requirements/pypi__ruamel_yaml_clib/ruamel.yaml.clib-0.2.0.dist-info
./jiko_runtime_requirements/pypi__ruamel_yaml_clib/ruamel.yaml.clib-0.2.0.dist-info/RECORD
./jiko_runtime_requirements/pypi__ruamel_yaml_clib/ruamel.yaml.clib-0.2.0.dist-info/LICENSE
./jiko_runtime_requirements/pypi__ruamel_yaml_clib/ruamel.yaml.clib-0.2.0.dist-info/namespace_packages.txt
./jiko_runtime_requirements/pypi__ruamel_yaml_clib/ruamel.yaml.clib-0.2.0.dist-info/WHEEL
./jiko_runtime_requirements/pypi__ruamel_yaml_clib/ruamel.yaml.clib-0.2.0.dist-info/top_level.txt
./jiko_runtime_requirements/pypi__ruamel_yaml_clib/ruamel.yaml.clib-0.2.0.dist-info/METADATA
./jiko_runtime_requirements/pypi__ruamel_yaml_clib/ruamel
./jiko_runtime_requirements/pypi__ruamel_yaml_clib/ruamel/__init__.py
./jiko_runtime_requirements/pypi__ruamel_yaml_clib/ruamel/yaml
./jiko_runtime_requirements/pypi__ruamel_yaml_clib/ruamel/yaml/__init__.py
./jiko_runtime_requirements/__init__.py
./jiko_runtime_requirements/pypi__ruamel_yaml
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel.yaml-0.16.10.dist-info
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel.yaml-0.16.10.dist-info/RECORD
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel.yaml-0.16.10.dist-info/LICENSE
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel.yaml-0.16.10.dist-info/namespace_packages.txt
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel.yaml-0.16.10.dist-info/WHEEL
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel.yaml-0.16.10.dist-info/top_level.txt
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel.yaml-0.16.10.dist-info/METADATA
./jiko_runtime_requirements/pypi__ruamel_yaml/__init__.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/__init__.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/scalarfloat.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/scanner.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/compat.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/error.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/anchor.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/constructor.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/composer.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/util.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/events.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/scalarint.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/scalarbool.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/configobjwalker.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/__init__.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/representer.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/tokens.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/dumper.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/cyaml.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/parser.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/reader.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/scalarstring.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/loader.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/resolver.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/py.typed
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/serializer.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/nodes.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/timestamp.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/main.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/emitter.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel/yaml/comments.py
./jiko_runtime_requirements/pypi__ruamel_yaml/ruamel.yaml-0.16.10-py3.8-nspkg.pth

The empty directories that are being imported:

./jiko_runtime_requirements/pypi__ruamel_yaml_clib/ruamel
./jiko_runtime_requirements/pypi__ruamel_yaml_clib/ruamel/__init__.py
./jiko_runtime_requirements/pypi__ruamel_yaml_clib/ruamel/yaml
./jiko_runtime_requirements/pypi__ruamel_yaml_clib/ruamel/yaml/__init__.py

Contents of both __init__.py files:

# __path__ manipulation added by rules_python_external to support namespace pkgs.
__path__ = __import__('pkgutil').extend_path(__path__, __name__)

from rules_python_external.

dillon-giacoppo avatar dillon-giacoppo commented on July 23, 2024

The logic that we have to create namespace directories based on namespace_packages.txt is wrong. Nothing in PEP-420 indicates that is required and it seems that file is an implementation detail of setuptools. Therefore, it does not particularly matter that ruamel.yaml.clib specifies that yaml is a namespace directory. We should remove the check pkg_resources_style_namespace_packages entirely. Since we only install from wheels, I don't believe we need to explicitly handle pkg-resources-style-namespace-packages anyway as it is implemented by non-empty __init__ files which will work out of the box in Bazel.

Additionally, if we strictly follow the PEP, we should revert #34. The correct behavior is to do nothing, not create an empty directory.

Fixing these will solve this issue although will not address the root cause (outlined below).

The creation of an empty directory with an __init__.py file that follows pkgutil-style namespace packages should still work. What is happening here is that ruamel.yaml is not actually a namespace package because __init__.py is non-empty and does not follow any of the namespace conventions. This is actually the same bug as #38. Essentially, these are not strictly namespace packages but leverage the behavior of implicit namespace packages to manually merge them together.

For example, twisted has an __init__.py with the following contents:

import os
import sys

def pluginPackagePaths(name):
    package = name.split('.')

    return [
        os.path.abspath(os.path.join(x, *package))
        for x
        in sys.path
        if
        not os.path.exists(os.path.join(x, *package + ['__init__.py']))]


__path__.extend(pluginPackagePaths(__name__))
__all__ = []                    # nothing to see here, move along, move along

These packages cannot be automatically converted to pkgutil-style namespace packages because for that requires every init file to have __path__ = __import__('pkgutil').extend_path(__path__, __name__) and they make assumptions on the lack of init files, e.g not os.path.exists(os.path.join(x, *package + ['__init__.py']))] from the above example.

The only way to achieve correct behavior here is to fix rules_python directly and have support for native implicit namespace packages. Essentially not implicitly creating __init__.py files everywhere.

Currently, this is configurable with the py_binary option legacy_create_init=False. Unfortunately, that option cannot be specified on just library targets so we would need to enforce that all of our rule consumers use the option on all their python targets or globally set it with the Bazel option --incompatible_default_to_explicit_init_py (which last time I checked breaks rules_docker). If implicit namespacing becomes supported than we can remove all of our custom namespace hacks as all the various mechanisms will be supported out of the box.

It would be good to patch this upstream @thundergolfer, either by completing the rollout of default_to_explicit_init_py (bazelbuild/bazel#7386) or by supporting legacy_create_init on py_library targets so we can selectively disable it on the targets created by this rule.

In the meantime, we can add an option to pip_install which disables namespace conversion from implicit to pkg_util style. This would assume consumers set either legacy_create_init=False or --incompatible_default_to_explicit_init_py.

from rules_python_external.

Related Issues (17)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.