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

Revert "Allow loading needles from pool directory" to fix loading time regression #1224

Merged
merged 1 commit into from Oct 10, 2019

Conversation

okurz
Copy link
Member

@okurz okurz commented Oct 10, 2019

@okurz okurz merged commit e2642e3 into master Oct 10, 2019
@okurz okurz deleted the revert-1217-consider-needles-in-pool-directory branch October 10, 2019 16:23
@coolo
Copy link
Contributor

coolo commented Oct 10, 2019

this is purely based on guess work, right?

# - Otherwise, the needle will be looked up under $bmwqemu::vars{PRJDIR}.
my @lookup_dirs;
my $needles_dir = $bmwqemu::vars{NEEDLES_DIR};
push(@lookup_dirs, $needles_dir) if (defined $needles_dir && index($needles_dir, cwd) == 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

this cwd call is what kills us. The whole if is pointless due to:

isotovideo:$bmwqemu::vars{NEEDLES_DIR} //= "$bmwqemu::vars{PRODUCTDIR}/needles";

As such we should move this cwd there. If NEEDLES_DIR is set but doesn't point to cwd, overwrite it.

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense, I'll test this tomorrow

Copy link
Contributor

@Martchus Martchus Oct 11, 2019

Choose a reason for hiding this comment

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

When I move the check to that assignment one can re-assign that variable later to sneak out from the working directory and load needles from everywhere. (With cwd this wasn't possible changing the working directory within autotest would not affect the backend.)

So I should do that check likely in needle::init where we also check that the needle directory exists. Then cwd would only be executed once per reload.

Copy link
Member Author

Choose a reason for hiding this comment

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

sounds useful. I was already surprised by that inefficiency when we looked at the code together yesterday :)

@okurz
Copy link
Member Author

okurz commented Oct 10, 2019

this is purely based on guess work, right?

@coolo no, bisected and tested with crosscheck, see
https://progress.opensuse.org/issues/57962#note-1

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.

None yet

3 participants