Skip to content

Duplicate branch list to avoid skipping entries#768

Merged
cottsay merged 1 commit into
masterfrom
cottsay/duplicate-list
Apr 22, 2026
Merged

Duplicate branch list to avoid skipping entries#768
cottsay merged 1 commit into
masterfrom
cottsay/duplicate-list

Conversation

@cottsay

@cottsay cottsay commented Apr 22, 2026

Copy link
Copy Markdown
Member

This loop is changing the list of branches while it's iterating over it. I can't explain why this hasn't been blowing up builds for the past decade, but at least on Python 3.14, this can result in skipping entries in the list when other entries are removed.

@cottsay cottsay self-assigned this Apr 22, 2026
@cottsay cottsay added the bug label Apr 22, 2026
@christophebedard

Copy link
Copy Markdown
Member

not to nitpick, but would .copy() convey the intention better?

This loop is change the list of branches while it's iterating over it. I
can't explain why this hasn't been blowing up builds for the past
decade, but on Python 3.14, this can result in skipping entries in the
list when other entries are removed.
@cottsay cottsay force-pushed the cottsay/duplicate-list branch from adadcf0 to 962aac6 Compare April 22, 2026 15:44
@cottsay

cottsay commented Apr 22, 2026

Copy link
Copy Markdown
Member Author

not to nitpick, but would .copy() convey the intention better?

It may convey intention better, yeah. That said I think it's a pretty standard practice. Looks like that was introduced in Python 3.3, which is relatively recent against the lifetime of Bloom, but we're only supporting back to Python 3.6 right now so it would be okay.

Not that duplicating our list of <20 strings would actually be a performance concern, but I did some light benchmarking and constructing a new (mutable) list using list.copy() was roughly 18% slower than creating a new (immutable) tuple (for a list of 6 strings). Pretty irrelevant here, but good to know.

@tfoote tfoote left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The copy indicator is definitely easier to read/maintain which is best for this use case.

Interesting that the tuple constructor is that much faster.

@christophebedard christophebedard left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not that duplicating our list of <20 strings would actually be a performance concern, but I did some light benchmarking and constructing a new (mutable) list using list.copy() was roughly 18% slower than creating a new (immutable) tuple (for a list of 6 strings). Pretty irrelevant here, but good to know.

Interesting!

@cottsay cottsay merged commit 1d1897b into master Apr 22, 2026
17 checks passed
@cottsay cottsay deleted the cottsay/duplicate-list branch April 22, 2026 16:08
@cottsay cottsay linked an issue Apr 30, 2026 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bloom-release failing on fedora rhel for some packages in ROS Lyrical

4 participants