Skip to content
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

Foreach: Order of repos make includes process before #356

Closed
raabf opened this issue May 16, 2022 · 6 comments · Fixed by #357
Closed

Foreach: Order of repos make includes process before #356

raabf opened this issue May 16, 2022 · 6 comments · Fixed by #357

Comments

@raabf
Copy link

raabf commented May 16, 2022

Environment

  • Output of tsrc version: tsrc 2.7.0
  • Operating system: OpenSuse Thumbleweed 20220420, Linux 5.17.3

This is a follow up problem of #352

Let us take this example:

repos:
  - url: git@git.example.org/chris 
    dest: chris
    branch: master

   # The repo bob depends on chris and requires it to be already installed, or it fails.
  - url: git@git.example.org/bob
    dest: bob
    branch: master

   # The repo alice depends on bob and requires it to be already installed, or it fails.
  - url: git@git.example.org/alice
    dest: alice
    branch: master


groups:

  core:
    repos:
      - chris
      - bob

  all:
    includes: [core]
    repos:
      - alice

With tsrc 2.7.0 the order is according to the group, so

so tsrc foreach --group core -j1 will process in the order “chris, bob" , which is OK.

If you do tsrc foreach --group core -j1 will process in the order “alice, chis, bob” (i.e. the elements in repos are processed before includes). However, I assume that includes mean that the group “all” needs group “core”, so that things in include are executed before the elements in repos. So I would expect that tsrc foreach --group core -j1 will process in the order “chirs, bob, alice” instead. Hence can we please process includes before (and other groups in the order as listed in includes) the repos?

@dmerejkowsky
Copy link
Collaborator

Sounds reasonable. To tell you the truth I did ask myself about how to handle the group inclusion when I had to make the tests pass.

Can you please take a look at the new tests on the following PR: #357 and tell me what you think ?

@raabf
Copy link
Author

raabf commented May 18, 2022

Sounds reasonable. To tell you the truth I did ask myself about how to handle the group inclusion when I had to make the tests pass.

Yea sorry I forgot that use-case and only noticed after I tried tsrc 2.7, I should have explained that before.

Can you please take a look at the new tests on the following PR: #357 and tell me what you think ?

I tested that branch, and my ten repositories, which depending on each other now works like a charm for tsrc foreach --group all -j1 -- pip install . I also looked at you unit tests with the assert actual ==, they make all sense to me now. In test_circle() things become weired of course when you have cyclic dependencies, but the resolving is good since it seems to begin at the repo a which was defined first. – Wonderful!

I notice a side effect:

groups:
  one:
    repos:
      - a
      - z

    two:
      - b
      - z

  all:
    includes: [one, two]
    repos:
      - c

If you run tsrc --group all -j1 foreach you get the order a, z, b, z, c, i.e. you got z twice. So I assume there is no duplicate elimination? In tsrc 2.6 you seem to have no duplicates, and I cannot image when you need to execute your command twice for the use cases of foreach. So I would expect that z is only executedo on its first occurene in the liniarization, i.e. a, z, b, c instead of a, z, b, z, c.

@dmerejkowsky
Copy link
Collaborator

dmerejkowsky commented May 18, 2022

So I assume there is no duplicate elimination?

There should be. We used to use unordered sets when processing group inclusion, which guarantees non-duplication, but in order to preserve the original order we now need to use lists instead.

Looks like a regression - we need more tests - and we need to fix the bug :P

@raabf
Copy link
Author

raabf commented May 19, 2022

Well at least one test with one duplicate ;-) – but probably that is already sufficient.

Yea that is probably one of the use cases why there are packages with an “OrderedSet” on pypi.org and like them – I ususally always workaround that by using continue if an element is reapeated in the iteration ...

@dmerejkowsky
Copy link
Collaborator

Can you take a look at : #357 ?

I think this works. I used a dict where the values don't matter - did not want to use a separate package just for that :)

@raabf
Copy link
Author

raabf commented May 30, 2022

did not want to use a separate package just for that :)

Yea for a single iteration, an additional package might be oversized, it was anyway a site note from me. I like your workaround with the dict!

Can you take a look at : #357 ?

I tested again and there are no duplicates any more! And my nine python projects, which I manage with tsrc, and which all depend on each other now works fine on installing! Thank you very much 😄 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants