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

Do we have this documented somewhere? #8

Closed
TriplEight opened this issue Nov 3, 2021 · 3 comments · Fixed by #26
Closed

Do we have this documented somewhere? #8

TriplEight opened this issue Nov 3, 2021 · 3 comments · Fixed by #26
Labels
documentation Improvements or additions to documentation

Comments

@TriplEight
Copy link
Contributor

Is there a source of truth for this job?
I mean, there are jobs: https://github.com/paritytech/substrate/blob/master/.gitlab-ci.yml#L582-L590

.check-dependent-project:          &check-dependent-project
  stage:                           build
  <<:                              *docker-env
  <<:                              *test-refs-no-trigger
  <<:                              *vault-secrets
  script:
    - git clone
        --depth=1
        "--branch=$PIPELINE_SCRIPTS_TAG"
        https://github.com/paritytech/pipeline-scripts
    - ./pipeline-scripts/check_dependent_project.sh
        paritytech
        substrate
        --substrate
        "$DEPENDENT_REPO"
        "$GITHUB_PR_TOKEN"

# Individual jobs are set up for each dependent project so that they can be ran in parallel.
# Arguably we could generate a job for each companion in the PR's description using Gitlab's
# parent-child pipelines but that's more complicated.

check-dependent-polkadot:
  <<: *check-dependent-project
  variables:
    DEPENDENT_REPO: polkadot

check-dependent-cumulus:
  <<: *check-dependent-project
  variables:
    DEPENDENT_REPO: cumulus

and https://github.com/paritytech/polkadot/blob/master/.gitlab-ci.yml#L275-L294

.check-dependent-project:          &check-dependent-project
  stage:                           build
  <<:                              *docker-env
  <<:                              *vault-secrets
  script:
    - git clone
        --depth=1
        "--branch=$PIPELINE_SCRIPTS_TAG"
        https://github.com/paritytech/pipeline-scripts
    - ./pipeline-scripts/check_dependent_project.sh
        paritytech
        polkadot
        --polkadot
        "$DEPENDENT_REPO"
        "$GITHUB_PR_TOKEN"

check-dependent-cumulus:
  <<: *check-dependent-project
  variables:
    DEPENDENT_REPO: cumulus
  1. all jobs are executed regardless of PR mentioning a Companion: #number. It's OK if they would just run to check if there is a companion and exit, why checking if a companion is not mentioned?
  2. why polkadot can have only cumulus companion? And why substrate has both polkadot and cumulus?
  3. what happens if I do a cross-reference like it was here: Update CI image to the latest rustc substrate#10142 and Update CI image to use the latest rustc polkadot#4200 cc Companion script fails if companion pr was merged #7. Or is it enough to have just one companion reference? According to the current CI jobs it doesn't make any sense to refer to substrate companion from polkadot.
@TriplEight TriplEight added the documentation Improvements or additions to documentation label Nov 3, 2021
@joao-paulo-parity
Copy link
Contributor

why polkadot can have only cumulus companion? And why substrate has both polkadot and cumulus?

Cumulus depends on both Substrate and Polkadot, while Polkadot only depends on Substrate

what happens if I do a cross-reference like it was here: Update CI image to the latest rustc substrate#10142 and Update CI image to use the latest rustc polkadot#4200

In that case the Polkadot PR's reference should have been ignored because Substrate is not one of its dependents. If it it did not behave this way, then the implementation is incorrect.

@joao-paulo-parity
Copy link
Contributor

all jobs are executed regardless of PR mentioning a Companion: #number

If a companion is not specified in the description, then it is assumed that the PR's branch does not break the dependency's master code, whatever that is. This assumption is validated through the check, hence why it does not bail out even if the companion is not specified.

@joao-paulo-parity
Copy link
Contributor

Is there a source of truth for this job?

A high-level behavior/motivation should be drafted, not only for this script, but the companion build system as a whole. I had to learn second-hand about how it worked on one of my first weeks at Parity because there was no mention whatsoever of "companion" during my onboarding process. The Substrate contributing guidelines explains companions from a usage perspective but I am not aware of any document explaining how and why this system works the way it does. To this day I still do not know who designed the companion build system or the reasoning behind some of its inherently flawed behavior.

An explanation attempt was made at paritytech/parity-processbot#327 but

  1. It did not gain much exposure and
  2. It involves some processbot-specific concerns which can be trimmed down from a document which specifically refers to the companion build system alone

A holistic explanation might also benefit the system in allowing for improvements suggestions since more people will be able to understand it.

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

Successfully merging a pull request may close this issue.

2 participants