Open kristos80 opened 2 months ago
Are you able to perhaps make a gist with what you have as a project? Could it be that /
is just interpreted as an absolute path start?
Sure.
Here's a root project with a submodule:
https://github.com/kristos80/test-root-project
The submodule: https://github.com/kristos80/test-submodule in my local setup contains a .git file with that content:
gitdir: ../.git/modules/test-submodule
If I now run vendor/bin/roave-backward-compatibility-check
from within the folder containing the submodule, I get the error: Directory "/test-root-project/test-submodule" is not a GIT repository.
Ah, I see what's happening :-)
Indeed, vendor/bin/roave-backward-compatibility-check
does not download git submodules, nor run any commands around them: it's just a much more complicated problem space.
This bit here is cloning your project to a temporary location, then performing a clean composer install
there to analyze it:
That does not include any kind of git
submodule handling.
I am sorry if I don't get it correctly, but I think we are talking about something else :-)
Indeed, vendor/bin/roave-backward-compatibility-check does not download git submodules, nor run any commands around them: it's just a much more complicated problem space.
There's no need to download any submodule, as I am not running the command from the root project but rather from within the submodule:
vendor/bin/roave-backward-compatibility-check
and not
as it never reaches that code
If I add this very crappy code (protect your eyes people) and run vendor/bin/roave-backward-compatibility-check
it gets executed correctly:
File /vendor/roave/backward-compatibility-check/src/Command/AssertBackwardsCompatible.php
// Instead of $sourceRepo = CheckedOutRepository::fr(Env\current_dir());
$sourceRepo = CheckedOutRepository::fromModuleFileOrPath(Env\current_dir());
File: /vendor/roave/backward-compatibility-check/src/Git/CheckedOutRepository.php
public static function fromModuleFileOrPath(string $path): self {
if($fromModuleFile = self::fromModuleFile($path)) {
return $fromModuleFile;
}
return self::fromPath($path);
}
private static function fromModuleFile(string $path): ?self {
if(Psl\Filesystem\is_file("$path/.git")) {
$content = Psl\File\read("$path/.git");
if(str_contains($content, "gitdir:")) {
$modulePath = trim(str_replace("gitdir:", "", $content));
$path = "$path/$modulePath";
return new self($path);
}
}
return NULL;
}
I hope I made myself clear!
@kristos80 ah, I see, but that's only the first problem then, no?
The problems are two, if I understand correctly:
$CWD/.git
and $CWD/composer.json
existInstead of building support for this into this particular project, I'd probably rather document the extension points to make this work, as this is really scratching the edges of what's needed.
$CWD/.git
and $CWD/composer.json
not being at the same level is something I am open for improving.@Ocramius Yes I am talking about the first problem exclusively. To be honest I cannot see how the second case could be solved, since modules are not part of the composer dependencies and can only be understood at the git level and it would require a vast amount of work as you say.
Now getting back to the first problem. Yes, the problem lies in the case where the .git folder is not at the same level with the composer.json
and instead there's a .git file which maps to the actual git folder.
From all the info I could gather around regarding git modules here are some (probable) facts about them:
gitdir: ../.git/modules/submodule-name
gitdir
contains always a relative or it can, also, be an absolute path, which makes things more complicated for the latter, especially as, in my case, you need to run everything inside a container, where the original files would contain values given from the host. So if there's to create a solution, it would work (relatively) easy only in the case that the .git file would contain a relative path to the actual git folder.*When a project contains submodules it's called 'superproject' in git terms: https://git-scm.com/docs/gitsubmodules
From the all the above if I were to add support for submodules, I would propose something like the previous code I sent:
gitdir: $ACTUAL_MODULE_PATH
and grab its value$ACTUAL_MODULE_PATH
is either relative or absolute$path
variable after some form of normalisation$path
to that valueCheckedOutRepository::fromPath($path)
methodNevertheless my case might be a very edgy one as I need to do testing (simultaneously) on two different levels at once:
So it is very convenient to have a container to test everything at once, instead of having to test everything independently especially when all modules are in development phase as well.
I would be happy to propose a PR, but I am not sure if I can compete your level :-)
@kristos80 before going in depth in implementation, it is valuable to represent this in a test case.
See https://github.com/Roave/BackwardCompatibilityCheck/tree/8.8.x/test/asset/located-sources
We'd probably create a dummy project in there, with no dependencies, but so that we can point our suit at it and verify that BC breaks are caught 🤔
@Ocramius sounds like a plan. I'll try my best to contribute in any way I can, as well
@kristos80 just to clear some expectations here: I'm unlikely able to implement the change myself in the next weeks.
What you can do to help out is starting with the e2e test case in a PR.
Worth perhaps replicating https://github.com/Roave/BackwardCompatibilityCheck/blob/92d33e416d59d316dc174ba675baeb54ffa9e483/test/unit/LocateSources/LocateSourcesViaComposerJsonTest.php#L27 with an example of nested composer.json
at first?
I think we can probably expand the installation source to recurse parent directories until .git
is found :thinking:
@Ocramius yes I can definitely do that in the next couple of days and if I need anything, I will ping you
@Ocramius tests in my mac have errors and failures.:
1) RoaveE2ETest\BackwardCompatibility\Command\AssertBackwardsCompatibleTest::testWillRunSuccessfullyOnNoBcBreaks
...
[BC] CHANGED: Value of constant TestArtifact\TheClass::UNCHANGED_CONSTANT changed from '/private/var/folders/s7/gkw_9kg564zcw9f7531j6xrr0000gn/T/api-compare-fc94ec8ccb35bf3e6656c092336f55de5260bec36634d07366a07/src' to '/private/var/folders/s7/gkw_9kg564zcw9f7531j6xrr0000gn/T/api-compare-e18dbfbf3a4e3d008adb7fec8ad5cc949bfc45326634d0737cebd/src'
1 backwards-incompatible changes detected
I guess the __DIR__
constant is calculated differently (due to the nature of tmp folder in OSX?) even if the revision code remains the same. If I change this to something less dynamic, like PHP_EOL
or PHP_SAPI
, it gets resolved.
1) RoaveTest\BackwardCompatibility\Git\GitCheckoutRevisionToTemporaryPathTest::testCheckoutDirectoryIsCorrect
Failed asserting that '/var/folders/s7/gkw_9kg564zcw9f7531j6xrr0000gn/T/api-compare-428327492a803b6e0c612b157a67a50a472754616634d27cb5b44' contains "/tmp/api-compare-".
This is due to the fact that OSX uses the $TMPDIR
which resolves to something like /var/folders/xl/84p38nhj405frmrkdpqb3v9c0000gn/T/
. If I remove the "/tmp" for the string getting compared, it gets resolved as well.
Should I include those in the PR as well?
@kristos80 yes please, a PR with the new file structure for the tests is already awesome 👍
Failed asserting that '/var/folders/s7/gkw_9kg564zcw9f7531j6xrr0000gn/T/api-compare-428327492a803b6e0c612b157a67a50a472754616634d27cb5b44' contains "/tmp/api-compare-".
This could be hardened via realpath()
around sys_get_tmp_dir()
in the test, but running in docker is sufficient to me, since I don't maintain OSX tests, and don't want to anyway (the workers are too slow/too few)
Hi there and thanks for this wonderful project.
It seems that submodules are not supported.
I have a directory structure as such
Where the
/plugin/.git
file contains the standard module entry:gitdir: ../.git/modules/plugin
If I try to run the
vendor/bin/roave-backward-compatibility-check
command from within theplugin
directory, I get the error:Directory "/plugin" is not a GIT repository
, because as far as I can understand/plugin/.git
in my case needs to be a directory and not a file.I can see that a possible patch would be to have a statement as such
$sourceRepo = CheckedOutRepository::fromModuleFileOrPath(Env\current_dir())
inAssertBackwardsCompatible::execute
where theCheckedOutRepository
code could include something as such:I could try to create a PR but I am not sure that the proposed approach fits in your current design