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.

Sign up for GitHub

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

Jump to bottom

EdkRepo: Add --tags flag to sync command #150

Closed
wants to merge 1 commit into from
Closed

EdkRepo: Add --tags flag to sync command #150

wants to merge 1 commit into from

Conversation

ashedesimone
Copy link
Contributor

Add support for fetching tags.

Additionally move messages indicating that the sync is starting for a repo to the begining of that process to improve messaging.

Fixes #134

@ndhaller
Copy link
Contributor

ndhaller commented Apr 3, 2023

This causes an installation error:

File "C:\Python38\lib\site-packages\edkrepo\commands\sync_command.py", line 82, in get_metadata
'help-text': sync_arguments.TAG_HELP})
NameError: name 'sync_arguments' is not defined
Unable to launch preferred entry point. Launching default entry point edkrepo.edkrepo_cli.py
Installation Error:

You may need to use 'arguments' instead of 'sync_arguments', and add 'TAG_HELP' to edkrepo/commands/arguments/sync_args.py

@ashedesimone ashedesimone force-pushed the 134 branch 3 times, most recently from 075b5a2 to 0dce084 Compare April 10, 2023 23:17
@ashedesimone
Copy link
Contributor Author

This causes an installation error:

File "C:\Python38\lib\site-packages\edkrepo\commands\sync_command.py", line 82, in get_metadata
'help-text': sync_arguments.TAG_HELP})
NameError: name 'sync_arguments' is not defined
Unable to launch preferred entry point. Launching default entry point edkrepo.edkrepo_cli.py
Installation Error:

You may need to use 'arguments' instead of 'sync_arguments', and add 'TAG_HELP' to edkrepo/commands/arguments/sync_args.py

I have verified that the latest version of this PR has the correct imports. Thanks for pointing this out.

kevinsun49
kevinsun49 previously approved these changes Apr 11, 2023
View reviewed changes
ndhaller
ndhaller suggested changes Apr 11, 2023
View reviewed changes
edkrepo/commands/sync_command.py
@@ -167,13 +171,20 @@ def run_command(self, args, config):
manifest_repo = manifest.general_config.source_manifest_repo
global_manifest_path = get_manifest_repo_path(manifest_repo, config)
for repo_to_sync in repo_sources_to_sync:
if not args.fetch:
ui_functions.print_info_msg(humble.SYNCING.format(repo_to_sync.root, repo.active_branch), header = False)
Copy link
Contributor

Choose a reason for hiding this comment

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

edkrepo sync
Error: local variable 'repo' referenced before assignment

Copy link
Contributor

Choose a reason for hiding this comment

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

If you change 'repo.active_branch' to "repo_to_sync.branch" on line 177 and 179, it won't fail for the case where the repo is on a tag or commit instead of a branch. The info message will say 'None branch' though, followed by the existing warning message that the repo is not on a branch.

Syncing Edk2Platforms to latest None branch...
No need to sync repo Edk2Platforms since it is in detached HEAD state

edkrepo/commands/sync_command.py
if not args.fetch:
ui_functions.print_info_msg(humble.SYNCING.format(repo_to_sync.root, repo.active_branch), header = False)
else:
ui_functions.print_info_msg(humble.FETCHING.format(repo_to_sync.root, repo.active_branch), header = False)
Copy link
Contributor

Choose a reason for hiding this comment

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

edkrepo sync --fetch
Error: local variable 'repo' referenced before assignment

edkrepo/commands/sync_command.py
@@ -22,7 +22,7 @@
from edkrepo.commands.edkrepo_command import EdkrepoCommand
from edkrepo.commands.edkrepo_command import SubmoduleSkipArgument, SourceManifestRepoArgument
import edkrepo.commands.arguments.sync_args as arguments
import edkrepo.commands.humble.sync_command as humble
import edkrepo.commands.humble.sync_humble as humble
Copy link
Contributor

Choose a reason for hiding this comment

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

fixed on main already. removed from patch file for testing.
7b03d01

edkrepo/commands/arguments/sync_args.py Outdated
OVERRIDE_HELP = 'Ignore warnings and proceed with sync operations.'
TAG_HELP = 'Enables tags to be pulled and TC_* tags to be removed. Using this flag will result in a significantly longer sync time.'
Copy link
Contributor

Choose a reason for hiding this comment

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

"and TC_* tags to be removed"
The code below does "git fetch --tags"; I don't think that will remove any tags unless --force is used. Update TAG_HELP string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Help string updated

edkrepo/commands/sync_command.py Outdated
ui_functions.print_info_msg(humble.SYNCING.format(repo_to_sync.root, repo.active_branch), header = False)
else:
ui_functions.print_info_msg(humble.FETCHING.format(repo_to_sync.root, repo.active_branch), header = False)

local_repo_path = os.path.join(workspace_path, repo_to_sync.root)
# Update any hooks
if global_manifest_directory is not None:
update_hooks(hooks_add, hooks_update, hooks_uninstall, local_repo_path, repo_to_sync, config, global_manifest_directory)
repo = Repo(local_repo_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

repo is assigned here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Move repo assignment and local path calculation to the start of the for loop

@ashedesimone
EdkRepo: Add --tags flag to sync command
03b0028 
Add support for fetching tags.

Additionally move messages indicating that the sync
is starting for a repo to the begining of that process
to improve messaging.

Fixes tianocore#134

Signed-off-by: Ashley E Desimone <[email protected]>
Reviewed-by: Kevin Sun <[email protected]>
@ndhaller
Copy link
Contributor

"edkrepo sync" and "edkrepo sync -u" failing on multiple cloned projects using main branch + this patch.

Traceback (most recent call last):
  File "C:\Python38\lib\site-packages\edkrepo\edkrepo_cli.py", line 183, in main
    command.run_command(command_name, parsed_args, config)
  File "C:\Python38\lib\site-packages\edkrepo\commands\composite_command.py", line 38, in run_command
    return command.run_command(args, config)
  File "C:\Python38\lib\site-packages\edkrepo\commands\sync_command.py", line 177, in run_command
    ui_functions.print_info_msg(humble.SYNCING.format(repo_to_sync.root, repo.active_branch), header = False)
  File "C:\Python38\lib\site-packages\git\repo\base.py", line 718, in active_branch
    return self.head.reference
  File "C:\Python38\lib\site-packages\git\refs\symbolic.py", line 272, in _get_reference
    raise TypeError("%s is a detached symbolic reference as it points to %r" % (self, sha))
TypeError: HEAD is a detached symbolic reference as it points to '[commit SHA]'
Error: HEAD is a detached symbolic reference as it points to '[commit SHA]'

@ndhaller
Copy link
Contributor

The humble.SYNCING message used to occur after a "repo.git.checkout(repo_to_sync.branch)" is done.
By moving the humble.SYNCING message earlier, the repo is still checked out to a combination, which can be a tag or a SHA instead of a branch.
I believe the new error is caused because humble.SYNCING is using repo.active_branch while the repo is still on a commit SHA or Tag.

Possibly you could replace the repo.active_branch in the info message with repo_to_sync.branch to avoid the error.

ndhaller
ndhaller reviewed Apr 12, 2023
View reviewed changes
edkrepo/commands/sync_command.py
repo.remotes.origin.fetch("refs/notes/*:refs/notes/*")
if args.tags:
repo.git.execute(['git', 'fetch', '--tags'])
Copy link
Contributor

Choose a reason for hiding this comment

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

may want to put "git fetch --tags" into a try/except block.
As is, if this line fails, the whole edkrepo sync commands stops, but we might be able to reduce it to a warning.

For example, if a user local tag conflicts with remote tag of the same name, GitCommandError exception occurs.
"raise GitCommandError(command, status, stderr_value, stdout_value)"
"! [rejected] tagname -> tagname (would clobber existing tag)"
"The git command: git fetch --tags failed to complete successfully with the following errors."

@ashedesimone ashedesimone deleted the 134 branch September 29, 2023 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ndhaller ndhaller ndhaller requested changes

@kevinsun49 kevinsun49 kevinsun49 left review comments

@nate-desimone nate-desimone Awaiting requested review from nate-desimone

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

"edkrepo sync" support for tags
3 participants
@ashedesimone @ndhaller @kevinsun49