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

CirrusCI change in config format #3825

Closed
wants to merge 65 commits into from
Closed

Conversation

SeanTAllen
Copy link
Member

I played around with the new possible config format for cirrus. This is the result that has all the functionality we have for Linux builds. Plus, it adds debug builds automatically without the need to maintain separate configurations.

What this code does it build the yaml that powers CirrusCI.

We could...

  • stick with what we have and not use this work
  • use this work to replace the linux setup in the cirrus setup leave the non-linux as is
  • move everything over to this format

at least for our linux setups, i can say that this is less error prone. there's pretty much no way to screw up the cache busting and forget to do it (its based on the docker image name and tag with this).

If we went forward with this, I would add a decent amount of comments to explain what is going on.

Please note this is not done, so there's incomplete portions of this related to nightly and release builds but it demonstrates more or less how it would look. I didn't want to invest more time without a discussion of "is this a good fit for us".

@SeanTAllen SeanTAllen added the needs discussion Needs to be discussed further label Aug 15, 2021
@SeanTAllen SeanTAllen requested a review from a team August 15, 2021 16:05
@SeanTAllen
Copy link
Member Author

The 4 jobs in this screenshot are the 4 created from the config in the PR.

image

Comment on lines +4 to +12
if env.get("CIRRUS_PR", "") != "":
return create_pr_tasks()

if env.get("CIRRUS_CRON", "") == "nightly":
return create_nightly_tasks()

cirrus_tag = env.get("CIRRUS_TAG", "")
re.findall(r'^\d+\.\d+\.\d+$', cirrus_tag)
return create_release_tasks()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decide which tasks to create. PR, Nightly, or Release.

re.findall(r'^\d+\.\d+\.\d+$', cirrus_tag)
return create_release_tasks()

def create_pr_tasks():
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is full and complete. Release and Nightly aren't yet.

tasks = []

pr_tasks = linux_tasks()
# release tasks
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"release" task here is release config

release.update(linux_pr_scripts().items())
tasks.append(release)

# debug tasks
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create a debug config test as well

t['triple_os']
)

# finish debug task creation
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

debug only runs if non-debug passed.


return tasks

def linux_tasks():
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would be where new tasks get added. anything in the list of maps below would have configuration for PR, nightly and release built.

'triple_os': 'linux-rocky8'
},
{ 'name': 'x86-64-unknown-linux-centos8',
'image': 'ponylang/ponyc-ci-x86-64-unknown-linux-centos8-builder:20210225',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we changed the builder image, it would be updated here.

note, no cache-buster value to update separately. this means you cant do what i have done before and forget to update it. the downside is, there is no way to manually bust the cache; however, we've never really needed to do that.

]

def linux_pr_task(name, image, triple_vendor, triple_os):
cache_key = hash.md5(fs.read('lib/CMakeLists.txt') + image)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the cache key is now the hash of the lib/CMakeLists.txt and the docker image name. definitely simpler than the current setup in .cirrus.yml

}
]

def linux_pr_task(name, image, triple_vendor, triple_os):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be renamed as its not just for pr.

Comment on lines +125 to +140
def linux_pr_scripts(config="release"):
return {
'configure_script': 'make configure config=' + config,
'build_script': 'make build build_flags=-j8 config=' + config,
'test_script': 'make test-ci config=' + config
}

def linux_nightly_scripts():
return {
'nightly_script': 'bash .ci-scripts/x86-64-nightly.bash'
}

def linux_release_scripts():
return {
'release_script': 'bash .ci-scripts/x86-64-release.bash'
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only "real" difference between the release, nightly, and pr is the script to run.

Comment on lines +112 to +115
'env': {
'TRIPLE_VENDOR': triple_vendor,
'TRIPLE_OS': triple_os
},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the env is only needed for nightly and release tasks but there's no harm in having it for pr as well.

it makes everything less complicated to have it here.

@SeanTAllen
Copy link
Member Author

Our .cirrus.yml is getting very big. I'm thinking of moving forward with this to cut down on a lot of repetition.

I'm planning on starting by only moving the X86 Linux PR tests over to this and adding a note in the .cirrus.yml for where to look for them.

Then as we have others that are complicated-ish to maintain (like some of the stress test ci, nightly builds, and release builds) moving those over as well.

SeanTAllen added a commit that referenced this pull request Jul 5, 2022
It was error prone so there's a slightly less error prone way
that we moved to as represented here.

To really make this not error prone, we'd need to move the the
system as seen in:

#3825
@SeanTAllen
Copy link
Member Author

I'm going to try resurrecting this because it will make our cirrus stuff less brittle.

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Jul 5, 2022
SeanTAllen added a commit that referenced this pull request Jul 8, 2022
It was error prone so there's a slightly less error prone way
that we moved to as represented here.

To really make this not error prone, we'd need to move the the
system as seen in:

#3825
@SeanTAllen SeanTAllen removed the discuss during sync Should be discussed during an upcoming sync label Jul 12, 2022
@SeanTAllen SeanTAllen closed this Aug 5, 2023
@SeanTAllen SeanTAllen deleted the less-cirrusci-resources branch August 5, 2023 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion Needs to be discussed further
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants