Test_dependencies are downloaded before applying patches, rather than after

Didn't this use to work before? Currently this means we cannot post patches on drupal.org that introduce new dependencies in tests, so in my opinion this is a bug.

I just ran into the same issue in #2692877: Add Search API support. I agree we should download the test_dependencies from the patched .info file rather than just from the file in git.

Drupal.org looks up dependencies based on a db table generated by project_dependency, and passes these forward with the test request. To the best of my knowledge, we've never been able to support testing of dependencies added in a patch ... only upon commit, with those dependencies added as a test_dependency in the project's .info file.

Calling this a feature request and moving it to Project Dependency. Feel free to correct as needed. :-)

Currently the immediate dependencies are stored when a release is made. When the dependencies are requested the full dependencies are retrieved. Thus the immediate dependencies for a release are static while the full dependencies can change when other projects are released. During the release process the code is checked out using Git and the info or info.yml files are parsed.

As I understand this request a release can be patched thus changing the info or info.yml files. Since the database tables do not change the dependencies do not reflect the changes in the patch. What are the implications of changing this behavior?

Recently I had to turn off logging in the part of the code for determining the full dependencies. It seems that DrupalCl hammers this code with the different variations for testing. Checking out the code using Git is the most time consuming part of computing dependencies. I would expect changing the code to checkout the code every time dependencies are requested would easily overwhelm the server.

Note that we are also using an older version of Git that does not support shallow repo cloning. We could reduce the amount of work for Git by upgrading but there is concern that a newer version of Git might break things.

So the obvious way of implementing this will not work. Perhaps someone has a suggestion for how this could be implemented?

Honestly I'm not sure I switched this to the correct project. Please bounce it if it's the wrong place.

The issue here is that if you patch the info file, you will change the dependencies. Given the way drupalci works, d.o should just parse out dependencies (and dependencies of dependencies) and send the job along to drupal_api with a list of repos.

The DB can be used for tertiary dependencies, but the top-level declared ones should be determined from the info file, since we can patch that.

At first I thought it might be the wrong project but I think it is somewhat correct. There has to be some method to flag that the project needs to be patched. But I do not think that would be sufficient. Project Dependency could check out the code using Git but how would it know how to get the patch? I think what might work is an override where the immediate dependencies can be passed in as arguments.

Project Dependency is based on the status of the git repos ... if/when we get per-issue repositories, it might make sense to add something here; but to try and manage dependencies on a per-patch basis sounds like a significant amount of extra complexity for the project_dependency project.

I think a more efficient solution would be to introduce a DrupalCI plugin which, if there are patches being applied, checks the 'modified files' property on the job's 'codebase' object - and if a module .info file has been modified, re-parses the info file and checks the dependency list against that which was passed into the job by drupal.org (and adds the checkout of any newly added dependencies to the job definition prior to execution).

Mile23: Thoughts? I'd suggest we bounce this back as a DrupalCI feature request.

if a module .info file has been modified, re-parses the info file and checks the dependency list against that which was passed into the job by drupal.org (and adds the checkout of any newly added dependencies to the job definition prior to execution).

My reason for moving this issue away from drupalci is that discovering module dependencies is d.o's job, or more specifically, not drupalci's job.

If drupalci were to do all that, then DCI_AdditionalRepositories would only be a suggestion and not a requirement, and drupalci would be the one that grows in complexity.

So figuring out which files have been changed and then re-determining the dependencies should be the responsibility of some part of the system before drupalci_api, so that the API call can set a requirement instead of a suggestion.

That way the testbot won't change requirements on you.

Maybe this should be a PIFT issue?

Discovering module dependencies is certainly drupal.org's job.

Applying a patch, and testing the result, is DrupalCI's job.

When a dependency is added by a patch, there is no way for drupal.org to know this unless it's performing checkouts, apply patches, and tracking what files are modified by each patch ... and this is certainly not drupal.org's job. The only way I see these additional dependencies being added by drupal.org is if PIFT were to provide a UI enhancement that allows the end user to manually specify additional dependencies when uploading the file (which isn't really a bad idea ... but will need some thought from a design perspective).

DCI_AdditionalRepositories uses a 'definition preprocessor' plugin to add additional checkout steps to the job definition (which then trigger git checkout commands during job execution). We also have 'variable preprocessor' plugins, which can be used to adjust the value of other DCI_* variables; but these are executed after the definition preprocessors - so we can't use this mechanism to add new dependencies to DCI_AdditionalRepositories. DCI_AdditionalRepositories remains essential, and separate from what we are talking about here.

The plugin I described above would need to run after the codebase has been created, the patch applied, and the 'modified files' property datafilled. Using the current framework, the only way to do this would be to have a 'DCI_Patch' definition preprocessor add a 'patch dependencies' build step at the end of the setup phase, and have the patch dependencies buildstep plugin perform the logic outlined above. I agree, this isn't very clean, and adds a fair bit of "magic" complexity to DrupalCI.

However, given the discussions that Ryan and I had around decoupling the codebase build from actual test execution, I think there would be value in introducing a new round of plugin processing at the end of the codebase setup ... I propose we break the current build-step plugins into two groups ('build step' and 'execution step') plugins, where one is responsible for setup/codebase tasks and the other is responsible for execution/container tasks. All of our composer_install, fetch, patch, etc. plugins would belong to the first category, while the container command plugins would belong to the second. We create a hard break between 'codebase generation' and 'job execution', and execute the setup/codebase plugins at the end of the codebase generation round ... which gives us a location where we can then look at the patched .info file, decide if there are any 'new' checkout requirements introduced by the patch, and then process those checkouts as needed.

We've already agreed that we all want a more explicit split between codebase setup and job execution during our architectural discussions back in January; and the above approach gives us that (albeit, perhaps a bit sooner than we anticipated during those talks).

In the meantime, adding a UI option where end users can specify additional project checkouts to include in DCI_AdditionalRepositories can unblock the OP in the shorter term.

When a dependency is added by a patch, there is no way for drupal.org to know this unless it's performing checkouts, apply patches, and tracking what files are modified by each patch ... and this is certainly not drupal.org's job.

Well, it's the job of whatever system is specifying the test. drupalci_testbot does not specify the test, it runs the test that is specified. That's why we made an API layer.

So: Patch the codebase, make a special case for two different methods of determining dependencies if info files have changed, be responsible for determining compatible versions... That all a lives in d.o already, but let's just duplicate it in drupalci?

I wouldn't have as much of a problem with this if it were in a library we could import in both d.o and drupalci, so we're at least working under the same set of rules. Does such a thing exist in PIFR? Drush doesn't look well-factored enough to just grab some code out, but I didn't look very hard.

In the meantime, adding a UI option where end users can specify additional project checkouts to include in DCI_AdditionalRepositories can unblock the OP in the shorter term.

Yah, that's called a 'patch.' :-) I think making a UI for this would be too complex a band-aid, and it would turn into SOP, and you still end up with the default dependencies in the SUT, where they shouldn't be.

So: Patch the codebase, make a special case for two different methods of determining dependencies if info files have changed, be responsible for determining compatible versions... That all a lives in d.o already, but let's just duplicate it in drupalci?

No ... continue to do dependencies as we do today, and add a new processing phase at the end of the 'codebase setup' phase which would let us do additional processing. That additional processing might be 'run composer', 'untar an archive', or whatever; and in this case, I'm suggesting that 'whatever' would be 'see *if* the patch affects the .info file, and in the 0.01% of cases where that's actually true, take additional steps to resolve the newly added dependency.

The alternative is that, instead of storing dependencies 'per release', project dependency would have to store dependencies 'per patch'. I'm familiar enough with project dependencies code (as well as the rest of the drupalorg_project stuff) to advise that we probably don't want to go there.

Yah, that's called a 'patch.' :-) I think making a UI for this would be too complex a band-aid, and it would turn into SOP, and you still end up with the default dependencies in the SUT, where they shouldn't be.

Well, you suggested maybe this should go in PIFT. The only way it makes sense in PIFT is as a place for users to manually add a project to append to the end of the calculated dependencies array as provided by project_dependency. Frankly put, that would be by far the simplest way to unblock this specific use case (basically supporting a user-specified addition to the PD output).

I do not think a GUI interface for specifying dependencies is too complicated because I already wrote such an interface #1591550: Dependency overrides. The problem was a lack of interest. While Project Dependency is much better at providing the right dependencies there still are some edge cases like patching .info files where such a GUI would be useful. I do not think it is preferable to automatic approaches but it does provide a way forward when a problem occurs rather than just throwing up our hands.

But a more compelling reason (at least to me) is the ability of for project maintainers to test their project with different versions of dependencies. Right now the only way to do this is to commit changes to dependencies to the .info file and run tests or to manually download different versions of dependencies and test locally. For this reason most projects do not specify version requirements in their dependencies and this results in software quality issues of bad versions of dependencies being used which may not be apparent until a problem occurs.

A few things:

For dependency management, the current state of affairs is that dependencies are precalculated on drupal.org after a release is made or a commit is added to a dev release. This is all handled by project dependency and those dependencies are then passed on to the testbots as instructions in the DCI_AdditionalRepositories variable.

What we are going to implement next: The codebase itself will be responsible for declaring which dependencies it needs inside of its composer.json file, and the testbots will resolve those dependencies using composer, which it already has to do for things that are not drupal modules.

The first step to solving that is the drupal.org composer façade, which is getting very close to complete. (https://www.drupal.org/project/project_composer).

Once we've given that enough exercise, we'll make changes to the testrunner/PIFT to use composer to pull in all of the required dependencies, including drupal modules, and we will no longer rely on project_dependency to inform PIFT directly of declared drupal dependencies in .info and .yml files (via DCI_AdditionalRepositories. Project dependency will still be informing the composer façade of the info/yml deps when the version_control_arrival() and the project_release_create_package() hooks are called.

So, we'll make changes to PIFT, and we'll make changes to the testrunner , and in the meantime we'll continue with the understanding that in order to test out adding a new dependency, a commit will be required on a dev release branch so that project_dependency can be aware of it.

Deployed the new drupalci today.

So the build process works as follows:

core is cloned from git
contrib projects are required using composer + the composer facade, which brings in any dependencies in the repo.
Patches are applied - If a patch modifies composer.json, that projects repository is moved to an ancillary workspace, added as a local composer repository, and we composer require that project again, getting the new dependencies.
Any 'require-dev' dependencies from the composer facade or from the ancillary workspace (as added by a patch) are then composer required as well.

What this means for testing:

If you wish to add a new dependency in a patch, you must have a composer.json that has your drupal and external dependencies expressed in it. And then if patches modify that composer.json, drupalci will build with the modified dependenices.

Happy Festivus.

I know this is a closed issue, but the problem just hit me in #2890778-5: Populate the scheduler date fields for content created by Devel Generate. I also had it before when adding Rules integration and wanting to test before making the commit. Scheduler does not have its dependencies in a composer.json file, we only have them in .info.yml

From #14

But a more compelling reason (at least to me) is the ability of for project maintainers to test their project with different versions of dependencies. Right now the only way to do this is to commit changes to dependencies to the .info file

From #15

in the meantime we'll continue with the understanding that in order to test out adding a new dependency, a commit will be required on a dev release branch

If I have to make a commit and push, without being able to test it first, I would like to get it right. So can anyone tell me if the following syntax is correct for adding the devel_generate sub-module into testing so that my new test for integration with devel_generate can be run.

test_dependencies:
  - devel:devel_generate

Thanks
Jonathan

Hate to post on a closed issue here, but it looks like we're running into this problem on #2163581: Add Services Resources - is this working with just changes to .info files on d7?

Hi Manuel Garcia,

From what I understand in #18 the new process is only for D8 projects which have a composer.json file. For D7 projects, or for D8 changes to dependencies in .info.yml no solution has been implemented, and you need to commit the file before you can see the effect of the changes.

Jonathan

Crap ok, was afraid that was going to be the case.... thanks @jonathan1055

Thats not exactly true. You can include a composer.json in d7 projects as well, and it works the same way as it does in d8.

If you wish to test changes to dependencies in a patch, you *must* supply a composer.json. There will never be a way to test those changes solely in .info or .info.yml files. The alternative to supplying a composer.json is to commit the dependency changes.

Thanks @Mixologic for clarifying.

@mixologic: Thanks for clarifying that, I might have to take the extra step to add a composer.json file to my main projects, just because we add tests relatively frequently.

Another point of clarification, you only have to do this if you are *adding new dependencies* to your modules that you need downloaded in order to test your modules. Adding new tests does not require this.

Thanks @mixologic. Sorry to have slightly got it wrong, but at least my comment caused a full and proper explanation ;-) Very useful for any new folk coming to this thread, too.

@mixologic: Yeah, I understand that, the few modules I heavily focus on tend to add new test dependencies fairly often so we hit this somewhat frequently. Again, thanks for the clarification.

I ran into a similar problem.

Scenario: there is a composer.json with a syntax error ([warning] Invalid json in module...) and a correct .info.yml with a "test_dependency" . The DrupalCI test results are green.

When you patch the composer.json syntax problem, the DrupalCI patch tests are failing, because the "test_dependency" are skipped are not loaded.

Solution: Adding the dependency for tests to the "require-dev" section in the composer.json patch, as described above, helped to get a green patch test result. So it is not just a problem when adding dependencies, it is also a problem when recovering from syntax errors.

If you have a composer.json in your project you should *always* have all of your test_dependencies in composer.json. So that solution is the default, intended behavior.

Sorry to post on a closed issue, but this is the nearest one I have found to my topic in question.

In #2951072: Provide Scheduler drush commands for Drush 9 I found that when a patch alters composer.json file but that file does not have the dependencies (because they were in .info.yml) then all the dependencies are reset and removed (because drupalCI is now respecting a patched composer.json). This is OK, once I figured what was happening, and I am now adding the dependencies into our composer.json - see #2985051: Patching composer.json shows we need to add module dependencies

Two questions:

1. Should I leave the .info.yml dependencies in place, thus having the dependency data in two files? I know it is duplication, but I think that for installs not using composer removing the dependencies from .info.yml would cause difficulties?
2. I now need to add dependences to a sub-module of the main module. The sub-module currently has its dependencies specified its own .info.yml file and does not have a composer.json yet, so my patch creates one. However as there was no pre-existing composer.json it does not seem to be picked up as changed when the new set of dependencies are calculated. Is this the expected behaviour? If so, I guess I need to commit a simple composer.json for the sub-module, then patch it to test the dependencies being added.

Thanks. If this post should be elsewhere, please say and I'll move / remove it.

Jonathan

1. Yes. The dependencies are used by both drupal and by composer. Drupal uses that dependency information for figuring out whether or not modules can be disabled, and composer uses that information to determine what to download. The best practice is to have it in both places.

2. Drupalci isnt yet sophisticated enough to detect a patched submodules' composer.json files. The assumption is that since we test entire projects in drupalci, any dependencies that are required by submodules should be listed as test_dependencies of the main module.

Thanks Mixologic, that's very helpful (sorry for the long delay in responding).

I did some more work in #2985051: Patching composer.json shows we need to add module dependencies and have now committed main modules test dependency (which is devel) in the main composer.json and the sub-modules actual dependencies (which are rules and the main module) in the sub-module's composer.json.

Drupalci isnt yet sophisticated enough to detect a patched submodules' composer.json

That's fine, I accept this. However, the slight inconsistency is that the sub-modules actual dependencies (not just test dependencies) are removed if the main module composer.json is patched. Then when the dependencies are re-evaluated the sub-module is ignored, so we get test failures because the required modules have not been re-added.

https://www.drupal.org/pift-ci-job/1231508 is an example of this. The sub-module requires Rules, which is loaded initially, then removed and not re-loaded. The sub-module's composer.json has require drupal/rules, this is not patched, it is in the committed codebase. Maybe when the dependencies are being re-calculated the sub-modules should be considered, just as they are on initial loading?

Jonathan

To add some more info, in case it helps anyone, we know that when composer.json is modified by a patch or MR then the dependencies are re-evaluated but only from the changed composer.json, any other dependencies added by an existing mymoule.info.yml are removed and not re-added. That's all fine. But it can lead to unexpected test failures when the MR is committed. I discovered this in Scheduler's preparation for D10 where test dependencies incompatible with D10 were removed from composer.json and the tests passed, but after committing, the branch test failed, because those dependencies had not also been removed from .info.yml.