fix circular dependency issue with resolving includes #31
Conversation
|
Reference PR #29 for discussion |
|
Ok, so just having a I liked the tree approach as well though, so it'd be good to keep that code around. @kobbled: one use-case we're not covering here is if you'd have multiple versions of the same package and have projects that depend on specific versions of packages. Right now whichever version is discovered / processed first will be used to resolve dependencies for all projects in the workspace, which might not be what we'd want (note that having multiple packages with the same name only works because of a limitation in Given how Karel works the situation I describe above is not likely to be something you'd want to try in an actual system, but it might be something to consider before merging this. I'll add some more comments in-line. |
| @@ -655,27 +655,27 @@ def resolve_includes(pkgs): | |||
| logger.debug("Resolving includes for: {}".format(', '.join(pkg_names))) | |||
|
|
|||
| for pkg in pkgs: | |||
| # TODO: watch out for circular dependencies | |||
| visited = set() | |||
There was a problem hiding this comment.
Would it make sense to have a per-workspace visited set? As opposed to the per-package one you have here now?
We can probably assume that everything in rossum is deterministic, but with a per-package set we run the risk of inconsistent sets of dependencies being used for different packages (ie: different versions).
I know I wrote in my comment that being able to spec different versions for dependencies might be a good thing, but in that case it should not be a side-effect of where the variable is initialised, but part of the logic.
There was a problem hiding this comment.
A visited-set per workspace may not be the right thing to use in that case. If another package in the source depends on the same packages, those packages will get overlooked in the package that is looked at last. for example just putting the visited set outside of the package loop yields:
### Hash ###################
Hash_dir = c:\Users\matt\Dropbox\Programming\Git\Ka-Boost\vendor\Hash\src
Hash_deps = KUnit Strings
Hash_include_flags = /I"c:\Users\matt\Dropbox\Programming\Git\Ka-Boost\vendor\Hash\src\include" /I"c:\Users\matt\Dropbox\Programming\Git\Ka-Boost\vendor\KUnit\src\include" /I"c:\Users\matt\Dropbox\Programming\Git\Ka-Boost\vendor\Strings\src\include" /I"C:\Users\matt\Dropbox\Programming\Git\Ka-Boost\lib\errors\src\include"
build $build_dir\hash.pc: ktrans_pc $Hash_dir\src\hash.kl
lib_includes = $Hash_include_flags
description = Hash :: src\hash.kl
build $build_dir\test_hash.pc: ktrans_pc $Hash_dir\src\test\test_hash.kl
lib_includes = $Hash_include_flags
description = Hash :: src\test\test_hash.kl
### KUnit ###################
KUnit_dir = c:\Users\matt\Dropbox\Programming\Git\Ka-Boost\vendor\KUnit\src
KUnit_deps = Strings
KUnit_include_flags =
build $build_dir\kunit.pc: ktrans_pc $KUnit_dir\src\kunit.kl
lib_includes = $KUnit_include_flags
description = KUnit :: src\kunit.kl
build $build_dir\test_kunit.pc: ktrans_pc $KUnit_dir\src\test\test_kunit.kl
lib_includes = $KUnit_include_flags
description = KUnit :: src\test\test_kunit.kl
Where kunit fails in ktransw.
Also, the way the package manifests are set now the depends package do not have any version association with them. The only thing I managed to do with the dependency tree code is to use the most current version if there were multiple versions found.
I think it is a good idea to open it up as an issue and try to tackle it in a future commit/PR. It will take a some tweaking to get the manifest into a format of :
"depends" : [
"Strings<=1.2.0"
]
if you think that is appropriate.
|
@kobbled: just letting you know I'm not ignoring your PR. Just overwhelmed with work at the moment. |
|
np. I completely understand how busy you must be. Pleased you are even taking time out to accommodate me with your repos. I've been afk from this for the last week so I'm not in a hurry. |
|
Took some time to get back to this, but thanks for the contribution @kobbled 👍 I'm not entirely sure this same approach shouldn't be used for other circular dependencies as well, but we can address those when they arise/are encountered. |
|
Yes, I need to get back on this too. I might have some more small additions for you. Next on my list is an ftp push, and adding ls files. |
No description provided.