Skip to content

Conversation

@tstellar
Copy link
Collaborator

This script will perform a bisect between two git commits first using the rpm builds from the nightly snapshots and then builds from source.

Copy link
Collaborator

@tuliom tuliom left a comment

Choose a reason for hiding this comment

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

I have 3 suggestions.
The code LGTM overall.

# Enable the copr repo that we want to test.
# FIXME: There is probably some way to do this via the python API, but I
# can't figure it out.
subprocess.run(["dnf", "copr", "enable", "-y", copr_fullname])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Try this:

base = dnf.Base()
base.read_all_repos()
base.fill_sack()

repos = base.repos.get_matching('copr:' + copr_project)
repos.enable()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This works, but you have to first download the .repo file from COPR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see the problem now. Calling dnf copr is a simpler API.

# Disable project so future installs don't use it.
# FIXME: There is probably some way to do this via the python API, but I
# can't figure it out.
subprocess.run(["dnf", "copr", "disable", "-y", copr_fullname])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Likewise, but use repos.disable() instead.

Copy link
Collaborator

@kwk kwk left a comment

Choose a reason for hiding this comment

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

Hey @tstellar I hope you don't mind that I had a couple of style comments.

Comment on lines +13 to +40
class CoprProject:
UNTESTED = 0
GOOD = 1
BAD = 2

def __init__(self, name: str):
self.name = name
self.index = -1
self._status = CoprProject.UNTESTED

def __lt__(self, other: Self) -> bool:
return self.name < other.name

@property
def commit(self) -> str:
return self._commit

@commit.setter
def commit(self, commit: str) -> None:
self._commit = commit

@property
def status(self) -> int:
return self._status

@status.setter
def status(self, status: int) -> None:
self._status = status
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this usage of an IntEnum that is a subclass of int can help here to make the code more "readable". Also, if you make your CoprProject a dataclass you can define defaults and access the members. Usually a name: str = "default" is enough but since you explicitly want to only order by name we have to say which field participates in the comparison operators like le. I hope this makes sense.

Suggested change
class CoprProject:
UNTESTED = 0
GOOD = 1
BAD = 2
def __init__(self, name: str):
self.name = name
self.index = -1
self._status = CoprProject.UNTESTED
def __lt__(self, other: Self) -> bool:
return self.name < other.name
@property
def commit(self) -> str:
return self._commit
@commit.setter
def commit(self, commit: str) -> None:
self._commit = commit
@property
def status(self) -> int:
return self._status
@status.setter
def status(self, status: int) -> None:
self._status = status
import dataclasses
import enum
@enum.unique
class CoprProjectStatus(enum.IntEnum):
UNTESTED = 0
GOOD = 1
BAD = 2
@dataclasses.dataclass(kw_only=True, order=True)
class CoprProject:
name: str = field(compare=True)
index: int = field(compare=False, default=-1)
status: CoprProjectStatus = field(compare=False, default=CoprProjectStatus.UNTESTED)
commit: str | None = field(compare=False, default=None)



def get_snapshot_projects(chroot: str | None = None) -> list[CoprProject]:
copr_client = copr.v3.Client.create_from_config_file()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, know that we have a function for this here as well.

self._status = status


def get_snapshot_projects(chroot: str | None = None) -> list[CoprProject]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please put a comment here? Does the function get all snapshot projects for a given chroot ordered by name.

continue
if chroot and chroot not in list(p.chroot_repos.keys()):
continue
projects.append(CoprProject(p.name))
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you accept my suggestion on the dataclass above you're gonna have to make use of kw arguments here:

Suggested change
projects.append(CoprProject(p.name))
projects.append(CoprProject(name=p.name))

Comment on lines +53 to +55
for idx, p in enumerate(projects):
p.index = idx
return projects
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you need an explicit index here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

m = regex.search(b["source_package"]["url"])
if m:
return m.group(1)
raise Exception(f"Could not find commit for {project_name}, {chroot}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
raise Exception(f"Could not find commit for {project_name}, {chroot}")
raise Exception(f"Could not find commit for {project.name}, {chroot}")

projects = get_snapshot_projects()
good_project = None
bad_project = None

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest you create this helper function to eliminate the need to repeat all non-changing arguments. You only change the good/bad commit parts, right?

Suggested change
def git_bisect_helper(good_commit:str, bad_commit=str):
return git_bisect(
repo,
good_commit,
bad_commit,
args.configure_command,
args.build_command,
args.test_command,
)

Comment on lines +197 to +204
return git_bisect(
repo,
args.good_commit,
p.commit,
args.configure_command,
args.build_command,
args.test_command,
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return git_bisect(
repo,
args.good_commit,
p.commit,
args.configure_command,
args.build_command,
args.test_command,
)
return git_bisect_helper(good_commit=args.good_commit, bad_commit=p.commit)

Comment on lines +221 to +228
return git_bisect(
repo,
p.commit,
args.bad_commit,
args.configure_command,
args.build_command,
args.test_command,
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return git_bisect(
repo,
p.commit,
args.bad_commit,
args.configure_command,
args.build_command,
args.test_command,
)
return git_bisect_helper(good_commit=p.commit, bad_commit=args.bad_commit)

Comment on lines +249 to +256
return git_bisect(
repo,
args.good_commit,
args.bad_commit,
args.configure_command,
args.build_command,
args.test_command,
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return git_bisect(
repo,
args.good_commit,
args.bad_commit,
args.configure_command,
args.build_command,
args.test_command,
)
return git_bisect_helper(good_commit=args.good_commit, bad_commit=args.bad_commit)

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