-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
feat(c): add license support for conan lock files #6329
Conversation
} | ||
} else if strings.HasPrefix(line, "license") { // cf. https://docs.conan.io/1/reference/conanfile/attributes.html#license | ||
// trim extra characters - e.g. `license = "Apache-2.0"` -> `Apache-2.0` | ||
license = strings.TrimSuffix(strings.TrimPrefix(line, `license = "`), `"`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this example in the doc.
from conan import ConanFile
class MyAndroidNDKRecipe(ConanFile):
name="my_android_ndk"
version="1.0"
def package_info(self):
self.conf_info.define("tools.android:ndk_path", "bar")
We probably need a regexp like name\s*=\s*"([^"]+)"
. I want to avoid using regular expressions wherever possible, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition, if a conanfile.py is small enough, applying regexp to the file content might be simpler and faster.
var (
namePattern = regexp.MustCompile(`^\s*name\s*=\s*"(?P<name>[^"]+)"$`)
licensePattern = regexp.MustCompile(`^\s*license\s*=\s*"(?P<license>[^"]+)"$`)
)
...
content, err := io.ReadAll(r)
if err != nil {...}
// Find name
if match := namePattern.FindSubmatch(content); len(match) > 0 {
name = string(match[namePattern.SubexpIndex("name")])
}
// Find license
if match := licensePattern.FindSubmatch(content); if len(match) > 0 {
license = string(match[licensePattern.SubexpIndex("version")])
}
I am not 100% sure which is faster as I have not compared it with processing one line at a time. I was just thinking out loud, and it's not a performance critical process. You can decide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name="my_android_ndk"
It looks as bug, but we can also parse this case.
We probably need a regexp like name\s*=\s*"([^"]+)". I want to avoid using regular expressions wherever possible, though.
I also don't trust regexp and try not to use them.
I think we can just remove all the spaces.
I am not 100% sure which is faster as I have not compared it with processing one line at a time.
In the files that I checked, name
and license
fields are placed in first 20-30 lines.
So I think we won't gain much time by doing this.
if a conanfile.py is small enough, applying regexp to the file content might be simpler and faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added removing extra spaces - 89003d6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks as bug, but we can also parse this case.
Why is it a bug? It does not break the Python semantics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't seen such cases. Also, all test cases use the format name = "foo"
. Therefore, I assumed that this was a bug or maybe just a typo in the documentation.
In any case, this is easy to solve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conanfile is written by hand, so any format is possible, isn't it? I think there is a customary preferred format, but it is legitimate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a customary preferred format, but it is legitimate.
You are right 👍
} | ||
} else if strings.HasPrefix(line, "license") { // cf. https://docs.conan.io/1/reference/conanfile/attributes.html#license | ||
// remove spaces before and after `=` (if used). e.g. `license = "Apache-2.0"` -> `license="Apache-2.0"` | ||
license = strings.ReplaceAll(line, " ", "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It introduces another bug with `license = "Apache or MIT".
if license != "" { | ||
break | ||
} | ||
} else if strings.HasPrefix(line, "license") { // cf. https://docs.conan.io/1/reference/conanfile/attributes.html#license |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be more strict. Otherwise, it matches other than the license line. That's why I suggested regexp this time.
https://github.com/conan-io/conan-center-index/blob/55703762e2ca0560fd44d9008e00e61a0a64b23c/recipes/zlib/all/conanfile.py#L90
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like we can't avoid using regexp...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible, I don't want to use regexp. I'm still considering a way to parse the license line without regexp, but...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
take a look this -https://go.dev/play/p/yWIioLCblQ9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for scanner.Scan() { | ||
line := strings.TrimSpace(scanner.Text()) | ||
|
||
if strings.HasPrefix(line, "name") { // cf. https://docs.conan.io/1/reference/conanfile/attributes.html#name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
|
||
func licensesFromCache() (map[string]string, error) { | ||
required := func(filePath string, d fs.DirEntry) bool { | ||
return filepath.Base(filePath) == "conanfile.py" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't conanfile.txt
used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cache dir contains only conanfile.py
files:
root@3fcddc7fe2e5:/app# find /root/.conan -name "conanfile.*"
/root/.conan/data/zlib/1.3.1/_/_/export/conanfile.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is conanfile.py
required? I'm wondering if we just don't find a project using conanfile.txt
as most projects use conanfile.py
. But we can handle conanfile.txt
if we find such a case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC only conanfile.py
contains attributes - https://docs.conan.io/2/reference/conanfile_txt.html#conanfile-txt
Therefore, we can't detect package name/license from conanfile.txt
files.
Then we don't need to parse the conanfile.txt files.
|
||
func licensesFromCache() (map[string]string, error) { | ||
required := func(filePath string, d fs.DirEntry) bool { | ||
return filepath.Base(filePath) == "conanfile.py" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is conanfile.py
required? I'm wondering if we just don't find a project using conanfile.txt
as most projects use conanfile.py
. But we can handle conanfile.txt
if we find such a case.
// e.g. `license = "Apache or MIT"` -> ` "Apache or MIT"` -> `"Apache or MIT"` -> `Apache or MIT` | ||
if name, v, ok := strings.Cut(line, "="); ok && strings.TrimSpace(name) == attributeName { | ||
attr := strings.TrimSpace(v) | ||
return strings.TrimPrefix(strings.TrimSuffix(attr, "\""), "\"") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strings.Trim seems better.
return strings.TrimPrefix(strings.TrimSuffix(attr, "\""), "\"") | |
return strings.Trim(attr, `"`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed in 0d67966
Description
Add license support for
conan
dependencies.Licenses are looked for in
conanfile.py
files from the conan cache directory.Related issues
Checklist