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

A new test for file existence inside a package (for the new recipe format) #57

Closed
wants to merge 3 commits into from

Conversation

wolfv
Copy link
Contributor

@wolfv wolfv commented Jun 15, 2023

No description provided.

```yaml
package_contents:
# checks for the existence of files inside $PREFIX or %PREFIX%
files:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
files:
prefix:

Given that the rest of the categories use the "base location" as key name, this might make more sense.

# checks for the existence of `mamba/api/__init__.py` inside of the Python site-packages directory
# NOTE: the user can/should also use Python import checks
site_packages:
- mamba.api
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that we can have direct access to site_packages, but I am not sure about the Python import path syntax. How can I make sure I have a data file in my Python package, for example? I'd say we stick to normal paths here, so this would be written like this:

Suggested change
- mamba.api
- mamba/api/__init__.py

After all, we have the imports: tests anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's a good point.

cep-20.3.md Outdated Show resolved Hide resolved

# searches for `$PREFIX/lib/libmamba.so` or `$PREFIX/lib/libmamba.dylib` on Linux or macOS,
# on Windows for %PREFIX%\Library\lib\mamba.dll & %PREFIX%\Library\bin\mamba.bin
lib:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about versioned libraries? The numbered suffixes are placed in different spots of the filename in linux and macos.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check what CMake is doing (they also have some "find_library" heuristic).

I also think it would be nice if one can add the version number, e.g. libmamba.0 (and it would find either libmamba.so.0 or libmamba.0.dylib but that would require some more logic in the string parsing (or we make it more explicit).

@jaimergp
Copy link
Contributor

I like this a lot! Left a few comments.

I am also wondering how we can test that some files were not added. It is common to have tests/ top-level packages accidentally packaged, and we could have something like:

package_contents:
  files:
     - ~tests  # or "!tests"

@wolfv
Copy link
Contributor Author

wolfv commented Jun 15, 2023

Great point! I think exclamation mark doesn't work in YAML, but tilde is fine.

Co-authored-by: jaimergp <[email protected]>
@wolfv
Copy link
Contributor Author

wolfv commented Jun 15, 2023

Are there other special locations, e.g. for R packages?

@dhirschfeld
Copy link

I am also wondering how we can test that some files were not added. It is common to have tests/ top-level packages accidentally packaged, and we could have something like:

package_contents:
  files:
     - ~tests  # or "!tests"

Maybe:

package_contents:
  includes:
    - mamba/api/
  excludes:
     - mamba/tests/

@wolfv
Copy link
Contributor Author

wolfv commented Jun 15, 2023

@dhirschfeld that is also nice, but then it becomes harder to negate the existence of a given shared library, include file, etc. I lean towards ~ as prefix for negation.

Copy link

@JeanChristopheMorinPerso JeanChristopheMorinPerso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense. Though, I'm wondering if it would be a god occasion to introduce a strict mode where the recipe has to define strictly which files should be included in the package similar to how it's done in the RPM land (https://src.fedoraproject.org/rpms/yaml-cpp/blob/rawhide/f/yaml-cpp.spec#_72). package_contents really makes me think of that.

Though, I'm not sure if it's really related to your PR or if we should have this discussion somewhere else.

## The specification

We propose a new "test" section (following the split of testing sections in this [CEP](https://github.com/conda-incubator/ceps/pull/56)) to test for package file existence.
The test are executed on the raw package contents (without installing them). That should make them very fast to execute, as well as less brittle vs. cmd.exe / bash scripts.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "without installing them" mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means that the tests only require the paths.json file. The package does not need to be extracted or linked into an environment.

@wolfv
Copy link
Contributor Author

wolfv commented Dec 6, 2023

@JeanChristopheMorinPerso the RPM spec that you linked also has this:

%{_includedir}/yaml-cpp/

So it seems that it similarly has a "glob" syntax to take an entire directory. But thanks for the pointer, I didn't know about that!

@wolfv
Copy link
Contributor Author

wolfv commented Dec 6, 2023

Some things that came up yesterday in the mamba meeting:

  • differentiating between shared and static libraries is currently not possible – should we add another field (static_libs) to find the existence (or negate the existence!) of static libraries in the package?
  • it came up that the comment about Windows dlls should be placed in bin and the comment may be wrong
  • for windows dlls we should do a strong test (e.g. make sure that both dll and lib file exist as a pair

@JeanChristopheMorinPerso now that I think about it it could be a great idea to have a strict_mode: true / false that means that all files in the package have to be matched by any of the rules.

@wolfv
Copy link
Contributor Author

wolfv commented Dec 6, 2023

Maybe strict_mode should be true by default ... :)

@h-vetinari
Copy link

strict_mode sounds cool!

for windows dlls we should do a strong test (e.g. make sure that both dll and lib file exist as a pair

In an ideal world, this would also check that the .lib file is actually an import library, and not a static library with the same name. 😇

@wolfv
Copy link
Contributor Author

wolfv commented Dec 6, 2023

@h-vetinari I just chatted with ChatGPT and looks like we can do that with Goblin and some inspection of the files.

@JeanChristopheMorinPerso
Copy link

JeanChristopheMorinPerso commented Dec 6, 2023

@JeanChristopheMorinPerso the RPM spec that you linked also has this:

%{_includedir}/yaml-cpp/

So it seems that it similarly has a "glob" syntax to take an entire directory. But thanks for the pointer, I didn't know about that!

They support a glob-style match, but I don't know if %{_includedir}/yaml-cpp/ necessarily does a recursive include or not.

@jezdez
Copy link
Member

jezdez commented Jun 17, 2024

Closing in favor of #84.

@jezdez jezdez closed this Jun 17, 2024
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.

6 participants