fix CustomPackage handling #897
Conversation
| /** | ||
| * Get the PATH required to use this package. | ||
| */ | ||
| abstract public static function getPath(): ?string; |
There was a problem hiding this comment.
I think here can return a default value PKG_ROOT_PATH . '/bin', rather than making an abstract function.
There was a problem hiding this comment.
PKG_ROOT_PATH . '/bin' doesn't exist. PKG_ROOT_PATH . '/$name/bin' is not guaranteed to exist. Both go and zig use non-standard locations for the binaries.
There was a problem hiding this comment.
If we only look at these two special packages, we don't seem to need to add this abstract method - these two getPath calls are both certain classes.
There was a problem hiding this comment.
Yes, but I would expect more packages to need to add a path to the PATH. We already have the abstract getEnvironment method because of the same assumption, so I believe it should be made here too. In case we don't need one, the method can return null, but it's a hint to us to think about whether the package needs a path, so I make it abstract.
There was a problem hiding this comment.
@crazywhalecc if this is a blocker for you, I'm okay to change it to a function that returns null by default. We should look into merging this PR, though, because at the moment spc craft doesn't work until this is merged.
There was a problem hiding this comment.
Ah I just on business trip and have almost no time to deal with other things. Will check it soon.
There was a problem hiding this comment.
I'm open to changes in this PR, but I might also remove or modify it in 3.0. I don't think it's a big deal since we haven't finalized vendor mode yet.
| /** | ||
| * Get the PATH required to use this package. | ||
| */ | ||
| abstract public static function getPath(): ?string; |
There was a problem hiding this comment.
I'm open to changes in this PR, but I might also remove or modify it in 3.0. I don't think it's a big deal since we haven't finalized vendor mode yet.
What does this PR do?