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

CEP: Recipe serialization in packages #74

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

wolfv
Copy link
Contributor

@wolfv wolfv commented Apr 13, 2024

target_directory: # optional target directory where the source should be extracted
```

### System tools
Copy link
Contributor

Choose a reason for hiding this comment

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

This is implicit, but I guess that this dictionary is a dict[str, str] one right? Tool name string to version string.

## Full Example

```yaml
recipe:

Choose a reason for hiding this comment

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

Just going to throwing this out there, are we worried about how these files scale? I can imagine some very complex multi-output, multi-platform recipes could produce very long rendered_recipe.yaml files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, each package only contains a rendered recipe for a single output, for a single platform. So it's not going to be crazy big. But lockfiles can indeed become relatively large. I think I would expect at most something like ~1 Mb. However, that should compress pretty well (except for the checksums). And the benefits are pretty huge (SBOMs, reproducibility, ...).

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 would, however, be an option to only store e.g. the URL + 1 hash. That would reduce the size quite decently. Happy to consider / discuss that.

Choose a reason for hiding this comment

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

Ah I missed that there was one file per output. I saw the build section and immediately thought those were individual outputs.

I don't have a strong opinion, I'm just pointing out this as a potential future headache. I know we have some large manifest-type files in conda world (looking at you, repodata.json) and I'm sure we all agree we don't want to repeat that pattern.

constrains: []
run_exports: null
finalized_sources:
- url: http://curl.haxx.se/download/curl-8.0.1.tar.bz2
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 could add a content hash here (as proposed recently in conda-build).

```yaml
- ${{ compiler('c') }}
# becomes
- compiler: c

Choose a reason for hiding this comment

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

Can we say that this is the "evaluated" form? In my mind this isn't an evaluated form, it's more a serialized form.

Choose a reason for hiding this comment

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

I thought ${{ compiler('c') }} got evaluated to (e.g.) gcc_linux-64? Where does compiler: c come from?

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 currently have an "intermediate" form in rattler-build. We need that for pin_subpackage because that needs to have the rest of the recipe rendered first.
However, for compiler we can make the function return the final value instead. It would have some other benefits (e.g. that we can use the compiler function in other contexts easily).

```yaml
- ${{ pin_subpackage('libtool', exact=true) }}
# becomes
- pin_subpackage:

Choose a reason for hiding this comment

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

What's the rational behind not fully resolving the value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The value is fully resolved later on in the resolved dependencies. Internally we keep it unresolved in the recipe to later resolve it during the execution.


- Type: object
- Description: Contains information related to hashing.
- `hash`: The hash value (e.g., `60d57d3`).

Choose a reason for hiding this comment

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

Does this contain the full build string? For example py312h7f4fdc5_0 or is it really just the hash (7f4fdc5)? If it contains only the hash, should there be a suffix entry too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this contains only the hash. There is a hash_prefix entry indeed. Hash and build-string are two different things :)

- `hash`: The hash value (e.g., `60d57d3`).
- `hash_input`: The input values used for generating the hash.
- `hash_prefix`: Prefix for the hash, which can include values like `py311`
based on the variant configuration.

Choose a reason for hiding this comment

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

Is this solely based on the variant or can it be influenced by build.string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the hash is used by default in the build string, but the build string cannot influence the hash directly.

- Description: Contains directory paths used during the build process.
- `host_prefix`: The host prefix directory with placeholders for the host
environment.
- `build_prefix`: The build prefix directory for the build environment.

Choose a reason for hiding this comment

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

What happens when merge_build_and_host_envs is True?

Choose a reason for hiding this comment

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

I feel like we're missing a CEP or, at a minimum, a reference to some other spec or documentation. This list of directories should correspond to the prescribed behaviors for the tools that process the package recipe.

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume merged build and host means that host_prefix == build_prefix, so both values are the same. Alternatively, host_prefix == null.

sha256: 9609163c14cd49ec651562838f38b88ed2d370e354af312ddc78c2be76c08d37
```

The following fields are available for each source:

Choose a reason for hiding this comment

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

Are they almost a 1 to 1 from CEP-14?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The finalized sources are almost but not exactly. We are adding the actual rev of the git repository (so that we can retrieve the actual commit when rebuilding).

```yaml
url: # URL to the source code
sha256: # sha256 hash of the downloaded file
md5: # md5 hash of the downloaded file

Choose a reason for hiding this comment

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

Are they mandatory? Should the tool that produce the packages add the field even if it's not set in the original recipe?

cep-recipe-serialization.md Outdated Show resolved Hide resolved
cep-recipe-serialization.md Outdated Show resolved Hide resolved

### System tools

This section records all system tools that were used during build execution and

Choose a reason for hiding this comment

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

How does conda-build or rattler-build know which tool was used? Should this be somewhat defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a used_build_tool.json file in the specification :)

@dholth
Copy link
Contributor

dholth commented May 10, 2024

Why not .json?
Do we still need info/recipe/meta.yaml ?

Comment on lines 5 to 6
We want to specify everything that goes into the final package in a standardized
format. This defines the structure and contents of the rendered_recipe.yaml file

Choose a reason for hiding this comment

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

We might want to avoid using the word "everything" here. From what I gathered below, this CEP covers what goes into the info/recipe directory of the output package(s), and not literally everything that goes into the package.


The rendered recipe YAML file is stored in the `info/recipe` folder of the final
package. Alongside the rendered recipe, we copy the entire "recipe folder".
If the original recipe was not named `recipe.yaml`, then we rename it inside the package.

Choose a reason for hiding this comment

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

If the original recipe was not named recipe.yaml, then we rename it inside the package.

Is there a reason to rename, rather than capture the build command that was used while preserving the original filename? As written, this also suggests the existing (v1) recipes that use meta.yaml would/should have said meta.yaml renamed to recipe.yaml in the output package, and that seems...odd?

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 could keep the recipe.yaml name. We should definitely have a consistent name for the rendered recipe (currently rendered_recipe.yaml). I don't have super strong feelings. I do like it if we can find the recipe in a consistent place though so that extracting / inspecting it is easy.

```yaml
- ${{ compiler('c') }}
# becomes
- compiler: c

Choose a reason for hiding this comment

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

I thought ${{ compiler('c') }} got evaluated to (e.g.) gcc_linux-64? Where does compiler: c come from?

Comment on lines 129 to 146
##### `target_platform`

- Type: string
- Description: The target platform for which the package is being built (e.g.,
`osx-arm64`).

##### `host_platform`

- Type: string
- Description: The host platform used for building the package. Usually
equivalent to `target_platform`, except when the `target_platform` is
`noarch`. If the `target_platform` is `noarch`, the `host_platform` is the
platform where the build is being executed.

##### `build_platform`

- Type: string
- Description: The platform used for building the package (e.g., `osx-arm64`).

Choose a reason for hiding this comment

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

If we are going to use cross-compilation language ("target", "build", "host"), we should spend some space to explain what each of these terms really mean. E.g., describing both host_platform and build_platform as "the platform used for building the package" is confusing.

##### `variant`

- Type: object
- Description: Dictionary with the variant configuration used for the build.

Choose a reason for hiding this comment

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

We should specify where such configuration comes from and what actually gets recorded. E.g., do the values come from conda_build_config.yaml, and if so, do we only preserve those keys that are actually used in processing this recipe, or do we replicate all values from the cbc.yaml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think for now it is fine to say "from the variant"? :)

Comment on lines +296 to +315
The list of `resolved` dependencies contains a dictionary with the following
fields (equivalent to `RepoDataRecord`):

```yaml
build: hb7217d7_0
build_number: 0
depends: []
license: GPL-2.0-or-later
license_family: GPL
md5: fe8efc3385f58f0055e8100b07225055
name: libtool
sha256: efe50471d2baea151f2a93f1f99c05553f8c88e3f0065cdc267e1aa2e8c42449
size: 408403
subdir: osx-arm64
timestamp: 1672362003513 # when available
version: 2.4.7
fn: libtool-2.4.7-hb7217d7_0.conda
url: https://conda.anaconda.org/conda-forge/osx-arm64/libtool-2.4.7-hb7217d7_0.conda
channel: https://conda.anaconda.org/conda-forge/
```

Choose a reason for hiding this comment

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

Is there a reason to repeat the contents for RepoDataRecord? Seems to me that the URL and SHA256 checksum of the package would be enough to recreate the various build environments.

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, I agree. sha hash and url should be enough. I think it might be interesting to include additional information (e.g. licenses) so that tools could report on what-went-into the package ... Of course, given that we still have access to the packages we could recreate this information from the repodata / packages, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there might be a few more arguments:

  • repodata changes over time and this is an accurate snapshot as how it was at the time
  • it makes it easier to extract a conda-lock file from the rendered recipe
  • there might be additioanl opportunities for tooling in the future (license scanning, ...)

sha256: # computed sha256 hash of the source (this is added if not in the source description)
patches: # list of patches that should be applied to the source
target_directory: # optional target directory where the source should be extracted
file_name: # optional file name

Choose a reason for hiding this comment

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

Wouldn't the filename be part of path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filename is more for renaming the file in the work directory. So yes, you could add a file_name argument if you want (and then it would be rendered).

git: # git URL
rev: # resolved commit hash at build time
depth: # optional depth to clone the repository
patches: # list of patches that should be applied to the source

Choose a reason for hiding this comment

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

We should clarify where these patches come from. (I suspect, relative to the top-level of the directory, but it should be explicitly stated.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

relative to the recipe directory, yeah.

```yaml
git: # git URL
rev: # resolved commit hash at build time
depth: # optional depth to clone the repository

Choose a reason for hiding this comment

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

Is this supposed to correspond to the --depth argument to git clone (which basically truncates history), or to (any) submodule clone/init operations?

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 this is to truncate history (conda-build has the same argument).

md5: # md5 hash of the downloaded file
file_name: # optional file name to rename the downloaded file
patches: # list of patches that should be applied to the source
target_directory: # optional target directory where the source should be extracted

Choose a reason for hiding this comment

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

See previous comments re: target_directory.

@wolfv wolfv changed the title add initial CEP for recipe serialization CEP: Recipe serialization in packages Jun 10, 2024
- Description: Contains settings related to the final package output.
- `archive_type`: The type of archive used for packaging (either `tar_bz2` or
`conda`).
- `compression_level`: The numerical compression level used for packaging
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just for fun

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's for perfectly reproducible builds (https://github.com/prefix-dev/reproducible-builds)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be a better idea to reproduce the data after compression? I also notice the number of threads has been removed? The zstandard version is recorded somewhere or implied by the tool version?

Copy link
Contributor

Choose a reason for hiding this comment

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

The number of threads doesnt influence the final artifact so we removed it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the ideal world we can re-create the compressed artifact bit-for-bit. Content hashes are also important. But for security reasons and so on I believe it's nicer to be able to really recreate the artifact. At least I believe that's what most people in reproducible-builds.org are trying :)

```yaml
- ${{ pin_subpackage('libtool', exact=true) }}
# becomes
- pin_subpackage:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The value is fully resolved later on in the resolved dependencies. Internally we keep it unresolved in the recipe to later resolve it during the execution.


The rendered recipe YAML file is stored in the `info/recipe` folder of the final
package. Alongside the rendered recipe, we copy the entire "recipe folder".
If the original recipe was not named `recipe.yaml`, then we rename it inside the package.
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 could keep the recipe.yaml name. We should definitely have a consistent name for the rendered recipe (currently rendered_recipe.yaml). I don't have super strong feelings. I do like it if we can find the recipe in a consistent place though so that extracting / inspecting it is easy.

cep-recipe-serialization.md Show resolved Hide resolved
sha256: # computed sha256 hash of the source (this is added if not in the source description)
patches: # list of patches that should be applied to the source
target_directory: # optional target directory where the source should be extracted
file_name: # optional file name
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filename is more for renaming the file in the work directory. So yes, you could add a file_name argument if you want (and then it would be rendered).

```yaml
git: # git URL
rev: # resolved commit hash at build time
depth: # optional depth to clone the repository
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 this is to truncate history (conda-build has the same argument).

fn: ca-certificates-2024.2.2-hf0a4a13_0.conda
url: https://conda.anaconda.org/conda-forge/osx-arm64/ca-certificates-2024.2.2-hf0a4a13_0.conda
channel: https://conda.anaconda.org/conda-forge/
run_exports:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@baszalmstra - we removed these here, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep updated this in 00ad601

fn: libzlib-1.3.1-h0d3ecfb_0.conda
url: https://conda.anaconda.org/conda-forge/osx-arm64/libzlib-1.3.1-h0d3ecfb_0.conda
channel: https://conda.anaconda.org/conda-forge/
run_exports:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here again

Copy link
Contributor

Choose a reason for hiding this comment

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

I updated them in 00ad601

- perl >=5.32.1,<6.0a0 *_perl5
host:
specs:
- source: raw
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this needs to be updated as well

Copy link
Contributor

Choose a reason for hiding this comment

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

I updated them in 00ad601

cep-recipe-serialization.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@wolfv wolfv left a comment

Choose a reason for hiding this comment

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

I just did another pass as well.

@wolfv
Copy link
Contributor Author

wolfv commented Jul 2, 2024

I changed this CEP a little - most of the CEP is now "implementation specific" and serves as documentation of what rattler-build does.

However, the CEP does require alternative build tools to implement a rendered recipe, and also requires a new file to be added to the package (info/used_build_tool.json).

@jezdez
Copy link
Member

jezdez commented Jul 2, 2024

@conda/steering-council

This vote falls under the "Enhancement Proposal Approval" policy of the conda governance policy,
please vote and/or comment on this proposal at your earliest convenience.

It needs 60% of the Steering Council to vote yes to pass.

To vote, please leave yes, no or abstain as comments below.

If you have questions concerning the proposal, you may also leave a comment or code review.

This vote will end on 2024-07-16, End of Day, Anywhere on Earth (AoE). This is an extended voting period due to summer holiday time in the Northern Hemisphere.

@jezdez jezdez added the vote Voting following governance policy label Jul 2, 2024
@baszalmstra
Copy link
Contributor

yes

@wolfv
Copy link
Contributor Author

wolfv commented Jul 2, 2024

Please use the following form to vote:

@xhochy (Uwe Korn)

  • yes
  • no
  • abstain

@CJ-Wright (Christopher J. 'CJ' Wright)

  • yes
  • no
  • abstain

@mariusvniekerk (Marius van Niekerk)

  • yes
  • no
  • abstain

@goanpeca (Gonzalo Peña-Castellanos)

  • yes
  • no
  • abstain

@chenghlee (Cheng H. Lee)

  • yes
  • no
  • abstain

@ocefpaf (Filipe Fernandes)

  • yes
  • no
  • abstain

@marcelotrevisani (Marcelo Duarte Trevisani)

  • yes
  • no
  • abstain

@msarahan (Michael Sarahan)

  • yes
  • no
  • abstain

@mbargull (Marcel Bargull)

  • yes
  • no
  • abstain

@jakirkham (John Kirkham)

  • yes
  • no
  • abstain

@jezdez (Jannis Leidel)

  • yes
  • no
  • abstain

@wolfv (Wolf Vollprecht)

  • yes
  • no
  • abstain

@jaimergp (Jaime Rodríguez-Guerra)

  • yes
  • no
  • abstain

@kkraus14 (Keith Kraus)

  • yes
  • no
  • abstain

@baszalmstra (Bas Zalmstra)

  • yes
  • no
  • abstain


```yaml
path: # path to the source
sha256: # computed sha256 hash of the source (this is only added if not in the source entry in the original recipe)

Choose a reason for hiding this comment

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

This can be prohitively expensive to compute

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, I think it's still worth it though. We might even add a content hash :)
But we could also add knobs to disable it.

Copy link
Contributor

@jaimergp jaimergp left a comment

Choose a reason for hiding this comment

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

Just a few typos I found.

├── index.json
├── used_build_tool.json // new file!
└── recipe
├── build.bat
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, this has changed from bld.bat?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep :)

cep-recipe-serialization.md Outdated Show resolved Hide resolved
- Description: Contains directory paths used during the build process.
- `host_prefix`: The host prefix directory with placeholders for the host
environment.
- `build_prefix`: The build prefix directory for the build environment.
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume merged build and host means that host_prefix == build_prefix, so both values are the same. Alternatively, host_prefix == null.

@wolfv
Copy link
Contributor Author

wolfv commented Jul 16, 2024

@marcelotrevisani @jakirkham @CJ-Wright @mbargull last chance to vote!

@chenghlee
Copy link

For the record: voted "no" because while I like the ideas presented in this CEP, I don't think the CEP as written is ready to be adopted as a specification. IMO, various unanswered questions/comments need to be addressed before adoption, and we should better separate behavioral specifications from implementation details.

@mbargull
Copy link
Member

I think it is good to have these parts of the build output/process specified which is why I support to have a proposal for it here.
I do agree with Cheng in that this is not yet ready to be put in, though, since the "Specification" section is a bit light in detail and the bigger part of the text, i.e., description of implementation details, are likely to change, so nothing we'd set in stone here.

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Wolf! 🙏

Think these are good improvements. Am voting yes with a few suggestions/questions (tried to use the checkbox, but had issues with it for some reason)

cep-recipe-serialization.md Outdated Show resolved Hide resolved
Comment on lines +31 to +36
```js
{
"name": "rattler-build", // string
"version": "0.14.0" // string
}
```
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it is worth extending this in the future with other dependencies (like the solver used, Jinja version, etc.), in which case we may want to nest these somehow

Comment on lines +128 to +132
```yaml
- ${{ pin_subpackage('some-pkg-a', lower_bound="x.x.x") }}
- ${{ pin_compatible('python', upper_bound="x.x.x") }}
- ${{ pin_compatible('numpy', upper_bound="3.2.1", lower_bound="1.2.3") }}
```
Copy link
Member

Choose a reason for hiding this comment

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

Think currently we use max_pin / min_pin with x.x.x. And use upper_bound / lower_bound with 1.2.3

https://docs.conda.io/projects/conda-build/en/latest/resources/variants.html#pinning-expressions

Choose a reason for hiding this comment

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

This is intentionally unified in the recent set of CEPs, please see this thread

Comment on lines +373 to +377
- pin_subpackage: some-pkg-a
lower_bound: x.x # not present if set to None
upper_bound: x.x.x # not present if set to None
exact: true # not present if false
spec: some-pkg-a >=1.0.0,<2
Copy link
Member

Choose a reason for hiding this comment

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

Am curious how these work together. We have exact and a more relaxed spec as well bounds. What takes precedence?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They should not be allowed together, right?

@mbargull
Copy link
Member

(tried to use the checkbox, but had issues with it for some reason)

@jakirkham
It's really odd that it doesn't like to accept your edit :/.
I checked "yes" on your behalf.

@jakirkham
Copy link
Member

Thanks Marcel! 🙏

Yeah this happened to me a couple times. So just used "approved" to mean the same thing. Think it is because I tried on my iPhone with GitHub Mobile (not sure why it can't check a box or why it makes an empty edit though 🤷‍♂️)

@wolfv
Copy link
Contributor Author

wolfv commented Jul 22, 2024

The vote is closed, and we have the following result:

Total voters: 15 (valid: 13 = 86.67%)

Yes votes (11 / 84.62%):

No votes (2 / 15.38%)):

Abstain votes (0 / 0.00%):

Not voted (2):

Invalid votes (0):

Thus we reached quorum and enough YES votes to mark this as accepted. 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vote Voting following governance policy
Projects
None yet
Development

Successfully merging this pull request may close these issues.