Skip to content

Conversation

@Williangalvani
Copy link
Contributor

This tags master with the vehicle's version.h version, so that it can be consumed in the following stages, generating a metadata folder for master with the current version number

@Williangalvani Williangalvani force-pushed the add_latest branch 3 times, most recently from 9a7f157 to c0ae965 Compare December 16, 2025 18:35
Copy link
Contributor

@ES-Alexander ES-Alexander left a comment

Choose a reason for hiding this comment

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

Seems to be generally working - nice!

Here are some suggestions to clean up the code and behaviour a bit :-)

Comment on lines +79 to +87
version_path = Path(f'{self.repository_path}/{vehicle_type}/version.h')
version_path2 = Path(f'{self.repository_path}/Ardu{vehicle_type}/version.h')
path = version_path if version_path.exists() else version_path2
Copy link
Contributor

@ES-Alexander ES-Alexander Dec 17, 2025

Choose a reason for hiding this comment

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

I don't love that this is duplicated in get_version_for_tag.

Comment on lines 87 to 90
if vehicle_type == 'Sub':
self.repository.create_tag(f'Ardu{vehicle_type}-{major}.{minor}.{patch}')
else:
self.repository.create_tag(f'{vehicle_type}-{major}.{minor}.{patch}')
Copy link
Contributor

@ES-Alexander ES-Alexander Dec 17, 2025

Choose a reason for hiding this comment

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

I think it's better to just create the base tag, and add 'Sub': 'Sub' to the valid_name_map (instead of having a special "Ardu" case for it).

That simplifies the tagging code, and future-proofs the parsing for if/when ArduSub ends up getting included in the combined ArduPilot releases.

@ES-Alexander
Copy link
Contributor

I am curious whether this will break things when the release gets tagged properly, given the existing mechanism to not re-generate existing files...

Copy link
Member

@patrickelectric patrickelectric left a comment

Choose a reason for hiding this comment

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

  File "/mnt/king/git/blue/ParameterRepository/./scripts/run_parsers.py", line 55
    major, minor, _ = self.get_version_from_version_h(file)
                                                           ^
IndentationError: unindent does not match any outer indentation level

@patrickelectric
Copy link
Member

Rebase over master

Copy link
Contributor

@ES-Alexander ES-Alexander left a comment

Choose a reason for hiding this comment

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

Noice :-)

'ArduCopter': 'Copter',
'ArduPlane': 'Plane',
'ArduSub': 'Sub',
'Sub': 'Sub',
Copy link
Contributor

Choose a reason for hiding this comment

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

By the (alphabetical) order of the others, I guess this should be at the bottom, after Rover?

@ES-Alexander
Copy link
Contributor

@patrickelectric I'm unsure how the groundskeeper is intended to behave when an ArduPilot tag is newer than the last generation from the parsers, but there have been code changes in this repo that are more recent than the tag. At the moment it seems to just skip the new tags because of that (but that's not the fault of this PR). We can potentially just wait a few hours for the next commit in ArduPilot to happen to confirm that side of things is working properly.

It does already seem to be generating the tags as expected though, and running without the groundskeeper seems to also generate the files as expected 👍

@ES-Alexander
Copy link
Contributor

I ended up doing some final fixes on this and including it in #16, partly so that the new Blimp inclusion could actually have a generation (since it has no stable tags yet).

That PR will close this one when it gets merged.

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 this pull request may close these issues.

3 participants