Do not complain about missing qmake if it can actually be found or is going to be built as part of Qt 5
ClosedPublic

Authored by ouwerkerk on Feb 18 2019, 7:33 PM.

Details

Summary

This is a partial fix for: bug #404486

Test Plan

Ran kdesrc-build with --include-dependencies qt5 kdesu and the tool proceeded beyond the point in bug #404486 (it attempts to configure/build module sets now)

Diff Detail

Repository
R365 kdesrc-build
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8504
Build 8522: arc lint + arc unit
ouwerkerk requested review of this revision.Feb 18 2019, 7:33 PM
ouwerkerk created this revision.
ouwerkerk edited the test plan for this revision. (Show Details)Feb 18 2019, 7:36 PM
ouwerkerk added a reviewer: mpyne.
ouwerkerk updated this revision to Diff 52009.Feb 18 2019, 8:27 PM
  • Include qtdir when adjusting build context environment (variables)

The only thing missing is to adjust Util & Application modules, specifically the sub _checkForEssentialBuildPrograms in Application to consider qtdir/kdedir as well as $PATH (absPathToExecutable).
Possibly using a copy of the absPathToExecutable which takes an additional second @paths positional argument (list).

ouwerkerk updated this revision to Diff 52012.Feb 18 2019, 9:21 PM
  • Consider and prefer using tools from qtdir/bin and kdedir/bin if available.

With the last change bug #404486 should be fully fixed.

Questions: arc has kind of mangled the first commit message. How to undo, or rather how to present the diff as a coherent thing made up of 3 distinct commits? Should I abandon this diff manually fix up commit messages and create a new one?
Also how to properly tag commit messages to include bug numbers so bugs.kde.org gets updated when once this lands?

ouwerkerk added inline comments.Feb 18 2019, 9:35 PM
modules/ksb/Module.pm
652

Unrelated but shouldn't we probably really check for whether the relevant $kdedir/XXX is actually contained with a given env var instead of just '/usr' ?

664

Unrelated but shouldn't we probably really check for whether the relevant $kdedir/XXX is actually contained with a given env var instead of just '/usr' ?

ouwerkerk updated this revision to Diff 52014.EditedFeb 18 2019, 9:48 PM

Rebased git history to get proper tags in commit messages.

ouwerkerk retitled this revision from Do not complain about missing qmake if it is going to be built as part of building Qt5. to Do not complain about missing qmake if it can actually be found or is going to be built as part of Qt 5.Feb 18 2019, 11:36 PM
mpyne requested changes to this revision.Feb 19 2019, 12:13 AM

Aside from the absPathToExecutable2 new function the updated logic is sound. You'll want to keep the check you added to call absPathToExecutable without the preferred paths if the program couldn't be found even after that change as well, so don't get rid of that extra call you added to account for when the preferred paths don't find the program being sought.

modules/ksb/Module.pm
652

Perhaps, but I'm also not aware of any direct equivalent to things like XDG_DATA_DIRS for /usr/share. 64-bit or 32-bit suffixes are the kind of thing that worry me more to be honest.

modules/ksb/Util.pm
57 ↗(On Diff #52014)

Rather than a separate absPathToExecutable2, I'd rather have the existing function be able to optionally take a list of paths as an additional parameter. Maybe something like:

sub absPathToExecutable
{
    my ($prog, @paths) = @_;
    @paths ||= split(/:/, $ENV{'PATH'});
    ...

This should retain existing behavior for compatibility while allowing other paths to be passed in for testing or other purposes.

This revision now requires changes to proceed.Feb 19 2019, 12:13 AM
ouwerkerk updated this revision to Diff 52093.Feb 19 2019, 6:20 PM

Rework absPathToExecutable

mpyne accepted this revision.Feb 20 2019, 3:41 AM

Good to go! If you have commit access you'll need to commit to the Gitlab instance rather than git.kde.org directly. In my case I'd just run git remote add gitlab git@invent.kde.org:kde/kdesrc-build.git first, git fetch gitlab to ensure it's fully downloaded, and then run something like git push gitlab master to push to the proper remote.

This revision is now accepted and ready to land.Feb 20 2019, 3:41 AM
ouwerkerk updated this revision to Diff 52173.Feb 20 2019, 7:18 PM

Update commit message to track corresponding kdesrc-build issue in Gitlab on invent.kde.org

This revision was automatically updated to reflect the committed changes.
This comment was removed by ouwerkerk.