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

Update include_multiple_projects_version and fixes around remote revision #43

Merged
merged 3 commits into from
Sep 17, 2024

Conversation

massonju
Copy link
Contributor

This PR contains several updates/fixes:

  • include_multiple_projects_version: update expected_version
  • return default value for _get_remote[url|revision]
  • check remote revision when getting project branch

In demo-manifest project, aosp_include_level_1.xml has been modified,
the revision has been moved to the remote entry.

We must update the expected version in the test:
test_include_multiple_projects_version

Signed-off-by: Julien Masson <[email protected]>
@massonju
Copy link
Contributor Author

Request review from @david-baylibre

Copy link
Collaborator

@david-baylibre david-baylibre left a comment

Choose a reason for hiding this comment

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

Thanks @massonju for the updates

Just a small comment on dict.get()

@@ -267,10 +267,10 @@ def __add_remote_revision(self, remote, rev):
self.__remote_revision[remote] = rev

def __get_remote_url(self, remote):
return self.__remote_url[remote]
return self.__remote_url.get(remote, None)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Default value is None if value isn't found in Dict, so no need for the None parameter:
https://docs.python.org/3/library/stdtypes.html#dict.get

I don't know Google repo's behaviour if no remote is defined for the project in the manifest. In any case, we use this in the check step only, to compute the manifest version, so it's fine with me.

Copy link
Contributor Author

@massonju massonju Sep 17, 2024

Choose a reason for hiding this comment

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

Default value is None if value isn't found in Dict, so no need for the None parameter:
https://docs.python.org/3/library/stdtypes.html#dict.get

Yes you're right I can remove the parameter

I don't know Google repo's behaviour if no remote is defined for the project in the manifest. In any case, we use this in the check step only, to compute the manifest version, so it's fine with me.

And yes actually that should not be possible, I will remove the change in this function


def __get_remote_revision(self, remote):
return self.__remote_revision[remote]
return self.__remote_revision.get(remote, None)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same thing with default None

@massonju massonju force-pushed the jmasson/fix-revision-remote branch from ae2a94f to 88c3b69 Compare September 17, 2024 12:29
When we call __get_remote_revision, we should return None if the key
is not found instead of raising an exception.

Signed-off-by: Julien Masson <[email protected]>
The project branch is set according to:
1. 'revision' entry in the xml
or
2. the remote revision of the project
or
3. default branch (retrieved by 'default' xml entry)

We added the condition 2. because if a project contains a remote
different than the default, we must get the revision according to this
remote and not the default.

Signed-off-by: Julien Masson <[email protected]>
@massonju massonju force-pushed the jmasson/fix-revision-remote branch from 88c3b69 to 7ac7496 Compare September 17, 2024 12:35
@makohoek makohoek merged commit ff64a5f into makohoek:main Sep 17, 2024
2 checks passed
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