BLIS 2.0 (another attempt)#43
Conversation
Signed-off-by: Michał Górny <mgorny@quansight.com>
|
Hi! This is the friendly automated conda-forge-linting service. I failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/19273386150. Examine the logs at this URL for more detail. |
a30b16d to
ecc65bc
Compare
|
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
20d7c62 to
a04f849
Compare
|
Yay, finally green. I'm thinking of also building an OpenMP variant per #34, but I'm not sure if it's better to do it on top of this PR, or get the version bump in first. |
h-vetinari
left a comment
There was a problem hiding this comment.
Congratulations and thanks a lot for getting this back to green. Just a few nits, basically LGTM! :)
| - if not exist %LIBRARY_LIB%\\blis.lib exit 1 | ||
| - if not exist %LIBRARY_LIB%\\libblis.lib exit 1 |
There was a problem hiding this comment.
| - if not exist %LIBRARY_LIB%\\blis.lib exit 1 | |
| - if not exist %LIBRARY_LIB%\\libblis.lib exit 1 | |
| # import lib for the DLL | |
| - if not exist %LIBRARY_LIB%\\libblis.lib exit 1 | |
| # static library | |
| - if not exist %LIBRARY_LIB%\\blis.lib exit 1 |
There was a problem hiding this comment.
It's the other way around. libblis.lib is the static library.
There was a problem hiding this comment.
That's horrible naming. We usually have
%LIBRARY_BIN%\foo.dll
%LIBRARY_LIB%\foo.lib
for a DLL + import-lib pair. Not only do the names not match here, but it's actually the static library that takes the "obvious" name?! Since we're at a major version boundary here, can we please come up with some better naming? Preferrably something simple and self-evident like
%LIBRARY_BIN%\blis.dll
%LIBRARY_LIB%\blis.lib
%LIBRARY_LIB%\blis-static.lib
There was a problem hiding this comment.
We usually have
%LIBRARY_BIN%\foo.dll
%LIBRARY_LIB%\foo.lib
Replace foo with blis and that's what we have.
but it's actually the static library that takes the "obvious" name?!
No, the obvious name is blis.lib which is what the import library take.
The naming scheme of vcruntime.lib being import library and libvcruntime.lib being the static library is a windows convention in the OS.
There was a problem hiding this comment.
Thanks for the discussion here! I'd like to have these decisions documented (with their rationale, and their rejected ideas), maybe as an informative CFEP or if we don't feel like having a vote, in the knowledge base. I also believe that decisions about conventions like this tend to be ignored when they are not checked by automation (e.g. some kind of linting rules).
I would also like to remind everyone of being kind to each other and provide constructive feedback even when there's disagreement. Sometimes the tone is unnecessarily aggressive and we are here to add value to the ecosystem.
There was a problem hiding this comment.
Do you mean beyond https://conda-forge.org/docs/maintainer/knowledge_base/#install-paths-and-naming-conventions? (though it sounds like that's not an unanimously followed convention)
There was a problem hiding this comment.
That's a good summary of what's being used, but I think we can encourage a certain style for new additions. A new contributor wanting to check how to name the libraries should get a clear understanding of what to do:
- Which library names are ok and should used preferentially?
- Which library naming conventions should not be used despite being already present?
There was a problem hiding this comment.
There's also the underlying question of how far do we want to go with changing things, that is:
- Should we follow upstream conventions, or rename files? Are there specific conditions when we should prefer one over the other (i.e. compatibility concerns)?
- Should we actually try to change upstream behavior?
- The least problematic part: if upstream doesn't currently support Windows at all, which conventions should we use when submitting patches?
My addition was really "informative" rather than "normative". Perhaps we should indeed try to set a specific standard, but that requires answering these questions, most importantly concerning compatibility. Though I suppose such discussion should take place elsewhere — on the conda-forge.github.io repository first, perhaps?
There was a problem hiding this comment.
Filed conda-forge/conda-forge.github.io#2659 for that.
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
It no longer does anything. Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Signed-off-by: Michał Górny <mgorny@quansight.com>
Fixes conda-forge#34 Signed-off-by: Michał Górny <mgorny@quansight.com>
|
Pending the decision on DLLs, I've added an OpenMP variant :-). |
Signed-off-by: Michał Górny <mgorny@quansight.com>
…5.11.17.12.05.48 Other tools: - conda-build 25.9.0 - rattler-build 0.49.0 - rattler-build-conda-compat 1.4.9
|
Restored the original library names, and added comments telling which file is which. |
|
@isuruf, is this good to go now? I'd like to start fighting |
Checklist
0(if the version changed)conda-smithy(Use the phrase@conda-forge-admin, please rerenderin a comment in this PR for automated rerendering)Closes #35
Closes #42