Uploading add-on with wheels ends in errors #158

New Issue

mnnxp · 2024-05-30T00:00:06+02:00

mnnxp commented

2024-05-30 00:00:06 +02:00

Hello here :)

I added wheels to the manifest and archive as instructed

Manifest:

...
wheels = [
    "./wheels/blake3-0.4.1-cp311-cp311-macosx_10_12_x86_64.whl",
    "./wheels/blake3-0.4.1-cp311-cp311-macosx_11_0_arm64.whl",
    "./wheels/blake3-0.4.1-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl",
    "./wheels/blake3-0.4.1-cp311-none-win32.whl",
    "./wheels/blake3-0.4.1-cp311-none-win_amd64.whl",
]

Single (.zip) expansion package:

cadbaselibrary-blender-master.zip
└─ cadbaselibrary-blender-master
    ├─ __init__.py
    ├─ blender_manifest.toml
    ├─ ...
    └─ wheels
         ├─ blake3-0.4.1-cp311-cp311-macosx_10_12_x86_64.whl
         ├─ blake3-0.4.1-cp311-cp311-macosx_11_0_arm64.whl
         ├─ blake3-0.4.1-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
         ├─ blake3-0.4.1-cp311-none-win32.whl
         └─ blake3-0.4.1-cp311-none-win_amd64.whl

No errors occurred when installing the add-on exactly from this archive and testing. Example of a successful installation:

However, when I try to upload the new version with wheels to the website, I get the following errors:

    A declared wheel is missing in the zip file, expected path: cadbaselibrary-blender-master/./wheels/blake3-0.4.1-cp311-cp311-macosx_10_12_x86_64.whl
    A declared wheel is missing in the zip file, expected path: cadbaselibrary-blender-master/./wheels/blake3-0.4.1-cp311-cp311-macosx_11_0_arm64.whl
    A declared wheel is missing in the zip file, expected path: cadbaselibrary-blender-master/./wheels/blake3-0.4.1-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
    A declared wheel is missing in the zip file, expected path: cadbaselibrary-blender-master/./wheels/blake3-0.4.1-cp311-none-win32.whl
    A declared wheel is missing in the zip file, expected path: cadbaselibrary-blender-master/./wheels/blake3-0.4.1-cp311-none-win_amd64.whl

Please tell me where I went wrong if there is no error on the website :)

Hello here :) I added wheels to the manifest and archive as [instructed](https://docs.blender.org/manual/en/dev/extensions/python_wheels.html) Manifest: ``` ... wheels = [ "./wheels/blake3-0.4.1-cp311-cp311-macosx_10_12_x86_64.whl", "./wheels/blake3-0.4.1-cp311-cp311-macosx_11_0_arm64.whl", "./wheels/blake3-0.4.1-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", "./wheels/blake3-0.4.1-cp311-none-win32.whl", "./wheels/blake3-0.4.1-cp311-none-win_amd64.whl", ] ``` Single (.zip) expansion package: ``` cadbaselibrary-blender-master.zip └─ cadbaselibrary-blender-master ├─ __init__.py ├─ blender_manifest.toml ├─ ... └─ wheels ├─ blake3-0.4.1-cp311-cp311-macosx_10_12_x86_64.whl ├─ blake3-0.4.1-cp311-cp311-macosx_11_0_arm64.whl ├─ blake3-0.4.1-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl ├─ blake3-0.4.1-cp311-none-win32.whl └─ blake3-0.4.1-cp311-none-win_amd64.whl ``` No errors occurred when installing the add-on exactly from this archive and testing. Example of a successful installation: ![screen.png](/attachments/52908f2c-af37-44f0-ab68-97ef030b6bfd) However, when I try to upload the new version with wheels to the website, I get the following errors: ![Screenshot from 2024-05-30 00-12-02.png](/attachments/4279de45-0ed1-4b6e-a79a-b84da2dcfafa) ``` A declared wheel is missing in the zip file, expected path: cadbaselibrary-blender-master/./wheels/blake3-0.4.1-cp311-cp311-macosx_10_12_x86_64.whl A declared wheel is missing in the zip file, expected path: cadbaselibrary-blender-master/./wheels/blake3-0.4.1-cp311-cp311-macosx_11_0_arm64.whl A declared wheel is missing in the zip file, expected path: cadbaselibrary-blender-master/./wheels/blake3-0.4.1-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl A declared wheel is missing in the zip file, expected path: cadbaselibrary-blender-master/./wheels/blake3-0.4.1-cp311-none-win32.whl A declared wheel is missing in the zip file, expected path: cadbaselibrary-blender-master/./wheels/blake3-0.4.1-cp311-none-win_amd64.whl ``` Please tell me where I went wrong if there is no error on the website :)

screen.png

215 KiB

cadbaselibrary-blender-master.zip

2.3 MiB

Screenshot from 2024-05-30 00-12-02.png

341 KiB

Oleg-Komarov self-assigned this 2024-05-30 10:27:49 +02:00

Oleg-Komarov commented

2024-05-30 11:44:57 +02:00

@dfelinto I think we need to update the documentation and remove the leading ./ from the wheels paths, making it just

wheels = [
  "wheels/lalala.whl",
...
]

This error happens because the validation code expects the wheel directory to be next to the manifest.toml, and it doesn't do any path expansion or interpolation when matching between filenames listed in the zip and the filenames listed in the manifest, so the match fails on that leading ./ part.
Doing a proper path expansion in this context is not always possible: how would you interpret a path that tries to jump out of the zip root, e.g. wheels = ["../../../../lalala.whl"]? So to keep it simple, we just append the path to the dirname of manifest.toml.

@ideasman42 since current manifest already works in blender, I wonder if it could be possible to craft a path either like ../../../abc or an absolute path /a/b/c to jump out of the zip file when looking for wheels (a potential security issue). Or if blender code has a special treatment for ./ and verifies that a manifest can't point outside the zip content.
Given that it's possible to get a manifest that is not validated by the server, this check should be present in blender itself as well.

@dfelinto I think we need to update the documentation and remove the leading `./` from the wheels paths, making it just ``` wheels = [ "wheels/lalala.whl", ... ] ``` This error happens because the validation code expects the wheel directory to be next to the manifest.toml, and it doesn't do any path expansion or interpolation when matching between filenames listed in the zip and the filenames listed in the manifest, so the match fails on that leading `./` part. Doing a proper path expansion in this context is not always possible: how would you interpret a path that tries to jump out of the zip root, e.g. `wheels = ["../../../../lalala.whl"]`? So to keep it simple, we just append the path to the dirname of manifest.toml. @ideasman42 since current manifest already works in blender, I wonder if it could be possible to craft a path either like `../../../abc` or an absolute path `/a/b/c` to jump out of the zip file when looking for wheels (a potential security issue). Or if blender code has a special treatment for `./` and verifies that a manifest can't point outside the zip content. Given that it's possible to get a manifest that is not validated by the server, this check should be present in blender itself as well.

mnnxp commented

2024-05-30 19:20:37 +02:00

@Oleg-Komarov thank you for the advice, it worked. The new archive with the add-on was installed in Blender without errors and was successfully uploaded to the site.

But I see that you are pointing out a security issue, should I close this issue?

@Oleg-Komarov thank you for the advice, it worked. The new archive with the [add-on](https://extensions.blender.org/add-ons/cadbase-library/) was installed in Blender without errors and was successfully uploaded to the site. But I see that you are pointing out a security issue, should I close this issue?

Oleg-Komarov commented

2024-05-30 19:24:52 +02:00

No, please don't close this, it needs a follow up to fix the docs for everybody, and I don't know how blender is processing this field during install, so not sure if the security concern is material or not. And thank you for the report! I forgot to mention it in my first reply.

👍 1

Dalai Felinto commented

2024-05-31 14:36:16 +02:00

@Oleg-Komarov supporting ./ as relative path was by design (Campbell's idea). I don't have a strong opinion either way, but I was (am still) happy to oblige to his design.

Oleg-Komarov commented

2024-05-31 14:51:47 +02:00

This raises the following questions:

is there a difference between declaring a wheel path as wheels/lalala.whl and ./wheels/lalala.whl? in which scenarios does this difference play a role?
do we need to handle a path like wheels/./lalala.whl? assuming it is equivalent to wheels/lalala.whl. if yes, then why do we need it?

If the answer to the above is "no" and there is no other reason that makes the leading ./ notation essential, then I think we should update the docs and not extend the validation rules for the reason mentioned in my initial comment above.

This raises the following questions: - is there a difference between declaring a wheel path as `wheels/lalala.whl` and `./wheels/lalala.whl`? in which scenarios does this difference play a role? - do we need to handle a path like `wheels/./lalala.whl`? assuming it is equivalent to `wheels/lalala.whl`. if yes, then why do we need it? If the answer to the above is "no" and there is no other reason that makes the leading `./` notation essential, then I think we should update the docs and not extend the validation rules for the reason mentioned in my initial comment above.

Campbell Barton commented

2024-06-03 13:13:13 +02:00

@Oleg-Komarov couldn't the validator simply strip the ./ before comparing?

There is no need to support wheels/./lalala.whl for e.g.

The rationale is:

blender_manifest.toml has different variables that reference paths (currently wheels & [build] paths) & may have more in the future.
There are times it's necessary to reference directories, in that case . (which is often written as ./) needs to be supported.
Although right now directory path references aren't used.
Other tools I use allow ./path as a relative path reference (CMake does for e.g.) so I don't see why blender's manifest needs to be especially strict in this regard.

@Oleg-Komarov couldn't the validator simply strip the `./` before comparing? There is no need to support `wheels/./lalala.whl` for e.g. The rationale is: - `blender_manifest.toml` has different variables that reference paths (currently `wheels` & `[build] paths`) & may have more in the future. - There are times it's necessary to reference directories, in that case `.` (which is often written as `./`) needs to be supported. Although right now directory path references aren't used. - Other tools I use allow `./path` as a relative path reference (CMake does for e.g.) so I don't see why blender's manifest needs to be especially strict in this regard.

Oleg-Komarov commented

2024-06-03 14:41:08 +02:00

I think we may be looking at this from non-overlapping perspectives, and that's why our base assumptions differ.

Manifest is an overloaded term, and it can combine these two perspectives:

description of the artifact (zip file content + metadata for attribution/indexing/etc)
instructions for processing the artifact: e.g. arguments to commands that will install the artifact

I focus on the first one, while you might be focusing on the second. Your example of CMake processing its argv hints at the second perspective.

From the first perspective:

Current implementation resolves all paths referenced in manifest as paths relative to the location of the manifest file in the zip archive.
This makes it possible to unambiguously reference any file packed into zip as long as it is located in or under the same directory as the manifest itself. And we don't expect any other valid locations:

addon.zip:
addon-dirname/__init__.py
addon-dirname/blender_manifest.toml
addon-dirname/wheels/lalala.whl
addon-dirname/any-other-dir/
invalid-unexpected-path

We can reference any directory in an unambiguous way by using its directory name (any-other-dir), except for the current directory (the one that contains blender_manifest.toml). As you say, there is no use case for referencing current directory in a variable, and I also don't see when it may be useful in a context of describing the zip content.

From the second perspective:

If we need to define any path-like variable that doesn't point to a file in the zip content, but has some other meaning, e.g. where to put a build target or what to pass as an argument to some other command, we won't be trying to match this variable against the zip content. These variables are out of scope for the problem in question.

The wheels variable involves both perspectives, so I suggest that we apply reasoning from both, not only from the second one:
in other words, let's not introduce multiple ways to describe the zip content, but only use one canonical way.

I think we may be looking at this from non-overlapping perspectives, and that's why our base assumptions differ. Manifest is an overloaded term, and it can combine these two perspectives: - description of the artifact (zip file content + metadata for attribution/indexing/etc) - instructions for processing the artifact: e.g. arguments to commands that will install the artifact I focus on the first one, while you might be focusing on the second. Your example of CMake processing its argv hints at the second perspective. From the first perspective: Current implementation resolves all paths referenced in manifest as paths relative to the location of the manifest file in the zip archive. This makes it possible to unambiguously reference any file packed into zip as long as it is located in or under the same directory as the manifest itself. And we don't expect any other valid locations: ``` addon.zip: addon-dirname/__init__.py addon-dirname/blender_manifest.toml addon-dirname/wheels/lalala.whl addon-dirname/any-other-dir/ invalid-unexpected-path ``` We can reference any directory in an unambiguous way by using its directory name (`any-other-dir`), except for the current directory (the one that contains `blender_manifest.toml`). As you say, there is no use case for referencing current directory in a variable, and I also don't see when it may be useful in a context of describing the zip content. From the second perspective: If we need to define any path-like variable that doesn't point to a file in the zip content, but has some other meaning, e.g. where to put a build target or what to pass as an argument to some other command, we won't be trying to match this variable against the zip content. These variables are out of scope for the problem in question. The `wheels` variable involves both perspectives, so I suggest that we apply reasoning from both, not only from the second one: in other words, let's not introduce multiple ways to describe the zip content, but only use one canonical way.

Oleg-Komarov commented

2024-06-03 14:58:03 +02:00

to respond directly to this:

couldn't the validator simply strip the ./ before comparing?

of course we could add this transformation, but I am trying to come up with an honest and detailed commit message for why we need it, and I can't come up with one, because we don't seem to have a use case for this.

adding complexity and more tests to the code base for stuff we don't really need seems counter-productive, if the trade-off is just fixing the docs. if the trade-off is fixing something nontrivial in blender - that's another story, but you would need to help me with details

to respond directly to this: > couldn't the validator simply strip the ./ before comparing? of course we could add this transformation, but I am trying to come up with an honest and detailed commit message for why we need it, and I can't come up with one, because we don't seem to have a use case for this. adding complexity and more tests to the code base for stuff we don't really need seems counter-productive, if the trade-off is just fixing the docs. if the trade-off is fixing something nontrivial in blender - that's another story, but you would need to help me with details

Dalai Felinto commented

2024-06-03 15:51:38 +02:00

I think Oleg has a really strong point on separating values which refer to the content inside the .zip, from values which refer to paths outside the zip.

Campbell Barton commented

2024-06-04 02:27:28 +02:00

@Oleg-Komarov agree WRT the two perspectives.

We can reference any directory in an unambiguous way by using its directory name (any-other-dir), except for the current directory (the one that contains blender_manifest.toml). As you say, there is no use case for referencing current directory in a variable, and I also don't see when it may be useful in a context of describing the zip content.

It may be useful in describing content for Blender, examples could be:

A directory to store presets.
A directory which is kept intact when upgrading the extension.

Referencing a directory (which could include the root directory) just seems like the kind of thing a manifest might easily do, so I don't see why we would commit to a convention that makes this awkward.

@ideasman42 since current manifest already works in blender, I wonder if it could be possible to craft a path either like ../../../abc or an absolute path /a/b/c to jump out of the zip file when looking for wheels (a potential security issue). Or if blender code has a special treatment for ./ and verifies that a manifest can't point outside the zip content.
Given that it's possible to get a manifest that is not validated by the server, this check should be present in blender itself as well.

Blender never extracts relative paths or absolute paths that point outside the intended destination paths: see docs for ZipFile.extract.

I think Oleg has a really strong point on separating values which refer to the content inside the .zip, from values which refer to paths outside the zip.

There are no paths outside the zip - all paths referenced are part of the file list which is to be included in the zip.
If someone tries to refers to paths outside the zip it should raise an error when building (if it doesn't already), and it will not be included when installing - which is the primary concern since anyone can craft a zip-file that referenced bad paths.

If we make path lists use different "rules" it complicates the mental model for the person maintaining blender_manifest.toml file because they need to remember there are different rules for different path lists, as far as I can see this is mostly annoying and going to trip them up.

@Oleg-Komarov agree WRT the two perspectives. > We can reference any directory in an unambiguous way by using its directory name (`any-other-dir`), except for the current directory (the one that contains `blender_manifest.toml`). As you say, there is no use case for referencing current directory in a variable, and I also don't see when it may be useful in a context of describing the zip content. It may be useful in describing content for Blender, examples could be: - A directory to store presets. - A directory which is kept intact when upgrading the extension. Referencing a directory (which could include the root directory) just seems like the kind of thing a manifest might easily do, so I don't see why we would commit to a convention that makes this awkward. ---- > @ideasman42 since current manifest already works in blender, I wonder if it could be possible to craft a path either like `../../../abc` or an absolute path `/a/b/c` to jump out of the zip file when looking for wheels (a potential security issue). Or if blender code has a special treatment for `./` and verifies that a manifest can't point outside the zip content. > Given that it's possible to get a manifest that is not validated by the server, this check should be present in blender itself as well. Blender never extracts relative paths or absolute paths that point outside the intended destination paths: see docs for [ZipFile.extract](https://docs.python.org/3/library/zipfile.html#zipfile.ZipFile.extract). ---- > I think Oleg has a really strong point on separating values which refer to the content inside the .zip, from values which refer to paths outside the zip. There are no paths outside the zip - all paths referenced are part of the file list which is to be included in the zip. If someone tries to refers to paths outside the zip it should raise an error when building (if it doesn't already), and it will not be included when installing - which is the primary concern since anyone can craft a zip-file that referenced bad paths. ---- If we make path lists use different "rules" it complicates the mental model for the person maintaining `blender_manifest.toml` file because they need to remember there are different rules for different path lists, as far as I can see this is mostly annoying and going to trip them up.

Oleg-Komarov commented

2024-06-04 10:30:11 +02:00

I do understand these examples:

A directory to store presets.
A directory which is kept intact when upgrading the extension.

but I don't understand the hypothetical part "which could include the root directory" - in these examples above variables wouldn't reference a root directory, would they?

I don't want to waste everybody's time and I'm ok to make the change as soon as we have a real or a very concrete plausible example that I can put in the commit message justifying the change.
The only real example we have so far is wheels where we are dealing with a list of files, not even directories.

If the reason is that blender tooling produces manifests like this and we don't want to change it - that's also a fine reason for me, but I need to know why I am making a change.

I do understand these examples: > A directory to store presets. > A directory which is kept intact when upgrading the extension. but I don't understand the hypothetical part "which could include the root directory" - in these examples above variables wouldn't reference a root directory, would they? I don't want to waste everybody's time and I'm ok to make the change as soon as we have a real or a very concrete plausible example that I can put in the commit message justifying the change. The only real example we have so far is `wheels` where we are dealing with a list of files, not even directories. If the reason is that blender tooling produces manifests like this and we don't want to change it - that's also a fine reason for me, but I need to know why I am making a change.

Dalai Felinto commented

2024-06-04 11:28:49 +02:00

@ideasman42 in the end of the day the rules are arbritary. For instance, why not to support "" ?

In that sense I think it is fine to have a strict ruleset and stick to it. Whether that ruleset expects "./" or not is a separate matter, as long as we have one clear rule.

I noticed that on the [build] example we have yet another convention, which is to use "/":

[build]
paths_exclude_pattern = [
  "/.git/"
  "__pycache__/"
]

I think it makes no sense to support these three ways of saying the same thing. And all in all I'm inclined for the simplest solution (no ./, no /).

@ideasman42 in the end of the day the rules are arbritary. For instance, why not to support "\" ? In that sense I think it is fine to have a strict ruleset and stick to it. Whether that ruleset expects "./" or not is a separate matter, as long as we have one clear rule. --- I noticed that on the [build] example we have yet another convention, which is to use "/": ``` [build] paths_exclude_pattern = [ "/.git/" "__pycache__/" ] ``` --- I think it makes no sense to support these three ways of saying the same thing. And all in all I'm inclined for the simplest solution (no ./, no /).

Campbell Barton commented

2024-06-04 12:15:43 +02:00

@ideasman42 in the end of the day the rules are arbritary.

This isn't arbitrary, it's fairly ubiquitous for packaging and build systems (node.js, cmake, pyproject.toml all support this). Although this is as result of them fully resolving the paths, they also support ./example/../path too I expect. Nevertheless, I don't see why we need to go out of our way to prevent fairly common relative path notation that works elsewhere.

For instance, why not to support "" ?

It's not a valid path reference and doesn't work (as far as I'm aware) in any mainstream build system.

In that sense I think it is fine to have a strict ruleset and stick to it. Whether that ruleset expects "./" or not is a separate matter, as long as we have one clear rule.

I noticed that on the [build] example we have yet another convention, which is to use "/":
[build]
paths_exclude_pattern = [
  "/.git/"
  "__pycache__/"
]
I think it makes no sense to support these three ways of saying the same thing. And all in all I'm inclined for the simplest solution (no ./, no /).

This is a red-herring paths_exclude_pattern is pattern matching not a list of literal paths, it's compatible with .gitignore since that's a well established method of pattern matching paths. see https://git-scm.com/docs/gitignore#_examples

> @ideasman42 in the end of the day the rules are arbritary. This isn't arbitrary, it's fairly ubiquitous for packaging and build systems (node.js, cmake, pyproject.toml all support this). Although this is as result of them fully resolving the paths, they also support `./example/../path` too I expect. Nevertheless, I don't see why we need to go out of our way to prevent fairly common relative path notation that works elsewhere. > For instance, why not to support "\" ? It's not a valid path reference and doesn't work (as far as I'm aware) in any mainstream build system. > In that sense I think it is fine to have a strict ruleset and stick to it. Whether that ruleset expects "./" or not is a separate matter, as long as we have one clear rule. > > --- > > I noticed that on the [build] example we have yet another convention, which is to use "/": > > ``` > [build] > paths_exclude_pattern = [ > "/.git/" > "__pycache__/" > ] > ``` > --- > > I think it makes no sense to support these three ways of saying the same thing. And all in all I'm inclined for the simplest solution (no ./, no /). This is a red-herring `paths_exclude_pattern` is pattern matching not a list of literal paths, it's compatible with `.gitignore` since that's a well established method of pattern matching paths. see https://git-scm.com/docs/gitignore#_examples

Dalai Felinto commented

2024-06-04 12:46:07 +02:00

For the records, I heard you both talked and agreed on something. I'm happy with whatever you both agree.

Oleg-Komarov referenced this issue

2024-06-06 14:44:51 +02:00

Manifest: transform path before checking against the zip file list #171

Oleg-Komarov referenced this issue from a commit

2024-06-06 14:48:16 +02:00

Manifest: transform path before checking against the zip file list (#171)

Oleg-Komarov commented

2024-06-06 14:52:18 +02:00

Support for leading "./" was added in #171 (see the explanation in the PR description) and deployed.
The examples from the docs are expected to work now, if they don't - it is a bug.

Closing this issue.

Support for leading "./" was added in #171 (see the explanation in the PR description) and deployed. The examples from the docs are expected to work now, if they don't - it is a bug. Closing this issue.

Oleg-Komarov closed this issue

2024-06-06 14:52:19 +02:00

Sign in to join this conversation.