Bug 89886

I've worked on this but forgot to upload a wip patch. But as we're going to remove qt-4.8 from trunk very soon, we will no longer need to identify the version of qt on the qt port name, and this will change a lot on my patch. So I will just wait until qt-4.8 to be removed to continue on this, or if anyone else wants to do it, feel free to do so. :)

Could you upload that patch, please?

Created attachment 154522 [details] Patch wip patch. it will change a lot when qt-4.8 get removed.

Created attachment 157667 [details] work in progress The unittests are working.

Created attachment 157737 [details] work in progress patch I removed the qt-4.8 specific lines from qt.py and qt_unittest.py. What else should we do in relation to qt-4.8 removal?

Comment on attachment 157737 [details] work in progress patch View in context: https://bugs.webkit.org/attachment.cgi?id=157737&action=review LGTM, but this is an informal review. Maybe you should get some feedback from dpranke. > Tools/Scripts/webkitpy/layout_tests/port/qt.py:57 > + #print("host: " + str(host) + " options: " + str(options) + " port_name " + str(port_name)) You can remove this comment line. :-) > Tools/Scripts/webkitpy/layout_tests/port/qt_unittest.py:140 > + port._version = "qt-5.0-wk2" This looks wrong, you're setting a private member of port outside it. Not sure of how you should do though, maybe modifying our make_port() to handle this by default, or maybe passing a parameter on it?

Created attachment 159944 [details] patch I made the suggested modifications. Could somebody take a look at the patch?

View in context: https://bugs.webkit.org/attachment.cgi?id=159944&action=review > Tools/Scripts/webkitpy/layout_tests/port/qt.py:57 > + version = cls.qt_version(host.executive) Why do we need the version here? > Tools/Scripts/webkitpy/layout_tests/port/qt.py:59 > + port_name = '' No need for this line. The lines below will set the port_name in this function. > Tools/Scripts/webkitpy/layout_tests/port/qt.py:110 > + version = 'qt-5.0-wk2' Why is this the same as the port_name returned by the determine_full_port_name function? Shouldn't this be simply '5.0'?

Either I don't understand this change or there is some confusion over what the port_name is supposed to capture. the full port name is supposed to be able to uniquely identify the full baseline search path. It looks like this is not the case with this path, i.e., if I specify "qt-5.0-wk1" then I may get the apple, win or, linux baselines depending on which platform I'm running on. Is that correct? It is okay for "qt-5.0-wk1" to be accepted as a shorthand for "figure out which platform I'm on", but that would be *input* to determine_full_port_name, not output. Also, are you intentionally getting rid of qt-4.8 here (or is this already unsupported)?

following a conversation w/ rafaelbrando on #irc, maybe it'll be helpful for to give a little more context about the design here ... The basic idea is that some tools (like garden-o-matic) need a way to be able to ensure a very specific configuration for a port, one that uniquely specifies all of the various attributes we care about (implementation, operation system, os version, wk1/wk2, release/debug etc.) In particular, we care the most about the attributes that will affect what baseline_search_path() is used. The theory behind determine_full_port_name is that we hand that routine a string (and an optional options argument) and can get a string back out that will uniquely determine that configuration, so when the outputted string is handed to port.__init__(), the code doesn't need to look at anything else to determine behavior (i.e., we don't check and should ignore what operating system we're running on, because we might need to be able to construct a mac qt port even if we're running on linux). [ Note that at the moment no port implements this theory completely, because no port encodes release/debug in the full port name, and the code probably wouldn't even work right if we tried. This is okay, because we don't have different baseline search paths for release vs. debug ]. So, it's okay fot "qt-5.0-wk1" to be *input* to determine full port name, (this basically says use qt 5.0, not 4.8, and use wk2, not wk2, but pick the os you're running on) but hence not output. Note that determine_full_port_name() is *only* used by the port factory to figure out what to pass to the port constructor. It should determine what port.name() returns. However, the port.name() by itself should not be used to determine anything else about the port's behavior; there are separate methods to determine behavior, like baseline_platform_dir(), baseline_version_dir(), version(), operating_system(), configuration(), etc. Once the port is constructed, callers should just use port.name() for debugging ...

(In reply to comment #10) > following a conversation w/ rafaelbrando on #irc, maybe it'll be helpful for to give a little more context about the design here ... > > The basic idea is that some tools (like garden-o-matic) need a way to be able to ensure a very specific configuration for a port, one that uniquely specifies all of the various attributes we care about (implementation, operation system, os version, wk1/wk2, release/debug etc.) In particular, we care the most about the attributes that will affect what baseline_search_path() is used. > > The theory behind determine_full_port_name is that we hand that routine a string (and an optional options argument) and can get a string back out that will uniquely determine that configuration, so when the outputted string is handed to port.__init__(), the code doesn't need to look at anything else to determine behavior (i.e., we don't check and should ignore what operating system we're running on, because we might need to be able to construct a mac qt port even if we're running on linux). > > [ Note that at the moment no port implements this theory completely, because no port encodes release/debug in the full port name, and the code probably wouldn't even work right if we tried. This is okay, because we don't have different baseline search paths for release vs. debug ]. > > So, it's okay fot "qt-5.0-wk1" to be *input* to determine full port name, (this basically says use qt 5.0, not 4.8, and use wk2, not wk2, but pick the os you're running on) but hence not output. > > Note that determine_full_port_name() is *only* used by the port factory to figure out what to pass to the port constructor. It should determine what port.name() returns. However, the port.name() by itself should not be used to determine anything else about the port's behavior; there are separate methods to determine behavior, like baseline_platform_dir(), baseline_version_dir(), version(), operating_system(), configuration(), etc. Once the port is constructed, callers should just use port.name() for debugging ... This clarifies a lot! Thanks. I think we've been on the wrong approach here. We should use "qt-{version}-{wk1,wk2}-{platform}-{release,debug}" for determine_full_port_name() and then we would need to change how we use the port name on those separated methods you've talked about (baseline paths, configurations, etc). Ossy, Balazs, what do you think?

I probably wouldn't do the "-{release,debug}" part, since no other port does. Also, I meant to note that one could (and some have) argue that using strings for this is a bad design. In part I agree, and this is partially what the TestConfiguration object was intended to do, but it turns out that strings have some advantages, and TestConfigurations have their own disadvantages ...

Comment on attachment 159944 [details] patch Would it perhaps make sense to keep this code around a little longer until qtwebkit-2.3 is out or stops merging from trunk? (this is an open question :) the alternative is that I just try to find commits like these and revert them in the branch to continue to be able to run the layout tests)

Comment on attachment 159944 [details] patch It is so obsolete. We will fix it later.

=== Bulk closing of Qt bugs === If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary. If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.