conda / conda-build

Commands and tools for building conda packages
https://docs.conda.io/projects/conda-build/
Other
381 stars 421 forks source link

Detect correct extraction for CDT binary rpms #3595

Open hmaarrfk opened 5 years ago

hmaarrfk commented 5 years ago

If a binary RPM only has one folder at the top level, say ./usr, then conda build will remove that folder from the structure. But for RPMs, we thus loose the intended folder structure.

Is there a way to stop top level folder stripping? Otherwise, maybe we can detect the intended folder structure and autodetect a fix like:

source:
  - url: http://mirror.centos.org/centos/6.10/os/x86_64/Packages/libudev-devel-147-2.73.el6_8.2.x86_64.rpm
    sha256: 318322d5e63b27bf423e011e9b3d8863e5b1aa5ac57de131add0a85c8e6bbbfe
    # The rpm only contains one folder, usr, so without this, we lose the
    # intended folder structure of the RPM
    folder: binary/usr

xref: https://github.com/conda-forge/staged-recipes/pull/8739#discussion_r300883633

Output of conda info
 conda info

     active environment : base
    active env location : /home/mark/miniconda3
            shell level : 1
       user config file : /home/mark/.condarc
 populated config files : /home/mark/.condarc
          conda version : 4.7.5
    conda-build version : 3.17.8
         python version : 3.7.3.final.0
       virtual packages : 
       base environment : /home/mark/miniconda3  (writable)
           channel URLs : https://conda.anaconda.org/conda-forge/linux-64
                          https://conda.anaconda.org/conda-forge/noarch
                          https://repo.anaconda.com/pkgs/main/linux-64
                          https://repo.anaconda.com/pkgs/main/noarch
                          https://repo.anaconda.com/pkgs/r/linux-64
                          https://repo.anaconda.com/pkgs/r/noarch
          package cache : /home/mark/miniconda3/pkgs
                          /home/mark/.conda/pkgs
       envs directories : /home/mark/miniconda3/envs
                          /home/mark/.conda/envs
               platform : linux-64
             user-agent : conda/4.7.5 requests/2.22.0 CPython/3.7.3 Linux/5.0.0-20-generic ubuntu/19.04 glibc/2.29
                UID:GID : 1000:1000
             netrc file : None
           offline mode : False
mbargull commented 5 years ago

I guess it would be fair to assume that RPM never have the typical root directory as is common for tarballs. Hence we can special-case the code for RPMs:

  1. Could change https://github.com/conda/conda-build/blob/3.17.8/conda_build/source.py#L160:
    -        if len(flist) == 1 and os.path.isdir(folder):
    +        if not src_path.lower().endswith('.rpm') and len(flist) == 1 and os.path.isdir(folder):
               hoist_single_extracted_folder(folder)
  2. or create a subfolder explicitly for .rpms in https://github.com/conda/conda-build/blob/3.17.8/conda_build/utils.py#L788.

The second suggestion might seem weird since it would create a folder just to remove it afterwards. If there is other code that uses conda_build.utils.tar_xf and might expect an archive root directory, it might make sense to do the special-casing on a lower level, i.e., tar_xf.

In the end, the choice of implementation comes down to where it seems more acceptable/maintainable to add ugly special handling stuff. I'll leave a PR for someone else to do as I'm not really experienced with the CDT workflow (and not keen to dig into test cases....).

mbargull commented 5 years ago

(cc. @jakirkham, @msarahan, @mingwandroid)

jakirkham commented 5 years ago

Thanks for raising @hmaarrfk! Also thanks for digging into this, @mbargull.

Sounds like 1 would be better. Though I haven't thought too much about this yet. Let's see what @mingwandroid says.

mbargull commented 5 years ago

Sounds like 1 would be better.

Yes, I'm inclined to agree since there is already the .whl handling a few lines above at https://github.com/conda/conda-build/blob/3.17.8/conda_build/source.py#L153 => having them next to each other means less littering all over the place ^^.

mingwandroid commented 5 years ago

Yeah, that's not good. I always disliked this aspect of CB.

I would rather we had a new metadata variable: source/strip_components, which defaults to 1, but for CDTs we set explicitly to 0. Pinging @msarahan.

mingwandroid commented 5 years ago

The name was stolen from tar. I think it's a reasonable choice.

mbargull commented 5 years ago

I'm definitely in favor of more general solutions akin to a source/strip_components. Thanks for the suggestion!

The name was stolen from tar. I think it's a reasonable choice.

It's not the clearest of names, however, I agree it's reasonable given packagers are usually familiar with tar and as such may recognize it.

github-actions[bot] commented 1 year ago

Hi there, thank you for your contribution!

This issue has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further activity occurs.

If you would like this issue to remain open please:

  1. Verify that you can still reproduce the issue at hand
  2. Comment that the issue is still reproducible and include:
    • What OS and version you reproduced the issue on
    • What steps you followed to reproduce the issue

NOTE: If this issue was closed prematurely, please leave a comment.

Thanks!

jakirkham commented 1 year ago

Let's keep this open