| *** tristan has quit IRC | 00:56 | |
| *** mohan43u has joined #buildstream | 05:42 | |
| *** benschubert has joined #buildstream | 07:24 | |
| *** jude has joined #buildstream | 07:27 | |
| *** coldtom4 has joined #buildstream | 08:12 | |
| *** benschubert has quit IRC | 08:12 | |
| *** coldtom has quit IRC | 08:12 | |
| *** milloni has quit IRC | 08:12 | |
| *** tpreston has quit IRC | 08:12 | |
| *** walterve[m][m] has quit IRC | 08:12 | |
| *** jswagner has quit IRC | 08:12 | |
| *** jward has quit IRC | 08:12 | |
| *** dbuch[m] has quit IRC | 08:12 | |
| *** segfault1[m] has quit IRC | 08:12 | |
| *** Trevio[m] has quit IRC | 08:12 | |
| *** krichter[m] has quit IRC | 08:12 | |
| *** Demos[m] has quit IRC | 08:12 | |
| *** tchaik[m] has quit IRC | 08:12 | |
| *** WSalmon has quit IRC | 08:12 | |
| *** jjardon has quit IRC | 08:12 | |
| *** pro[m] has quit IRC | 08:12 | |
| *** cgm[m] has quit IRC | 08:12 | |
| *** awacheux[m] has quit IRC | 08:12 | |
| *** skullone[m] has quit IRC | 08:12 | |
| *** DineshBhattarai[m] has quit IRC | 08:12 | |
| *** asingh_[m] has quit IRC | 08:12 | |
| *** jjardon[m] has quit IRC | 08:12 | |
| *** SamThursfield[m] has quit IRC | 08:12 | |
| *** kailueke[m] has quit IRC | 08:12 | |
| *** mattiasb has quit IRC | 08:12 | |
| *** dylan-m has quit IRC | 08:12 | |
| *** reuben640[m] has quit IRC | 08:12 | |
| *** theawless[m] has quit IRC | 08:12 | |
| *** connorshea[m] has quit IRC | 08:12 | |
| *** abderrahim[m] has quit IRC | 08:12 | |
| *** m_22[m] has quit IRC | 08:12 | |
| *** albfan[m] has quit IRC | 08:12 | |
| *** tlater[m] has quit IRC | 08:12 | |
| *** ironfoot has quit IRC | 08:12 | |
| *** benbrown has quit IRC | 08:12 | |
| *** lantw44 has quit IRC | 08:12 | |
| *** Trevinho has quit IRC | 08:12 | |
| *** jjardon_ has joined #buildstream | 08:12 | |
| *** benschubert has joined #buildstream | 08:13 | |
| *** benbrown has joined #buildstream | 08:14 | |
| *** ironfoot has joined #buildstream | 08:14 | |
| *** milloni has joined #buildstream | 08:15 | |
| *** tpreston has joined #buildstream | 08:19 | |
| *** jward has joined #buildstream | 08:19 | |
| *** WSalmon has joined #buildstream | 08:19 | |
| *** jswagner has joined #buildstream | 08:28 | |
| *** tpollard has joined #buildstream | 08:37 | |
| *** santi has joined #buildstream | 08:43 | |
| *** cphang has joined #buildstream | 08:57 | |
| juergbi | WSalmon: you aren't working on a test for https://gitlab.com/BuildStream/buildstream/-/merge_requests/1926, right? I can probably handle that today | 08:57 |
|---|---|---|
| *** lantw44 has joined #buildstream | 09:09 | |
| *** Trevinho has joined #buildstream | 09:09 | |
| *** SotK has joined #buildstream | 09:29 | |
| *** tristan has joined #buildstream | 09:46 | |
| *** Trevio[m] has joined #buildstream | 09:50 | |
| *** albfan[m] has joined #buildstream | 10:50 | |
| *** awacheux[m] has joined #buildstream | 11:00 | |
| *** coldtom4 is now known as coldtom | 11:28 | |
| *** reuben640[m] has joined #buildstream | 11:31 | |
| *** walterve[m][m] has joined #buildstream | 11:37 | |
| *** ironfoot is now known as ironfoot_ | 11:38 | |
| *** ironfoot_ is now known as ironfoot | 11:38 | |
| *** ChanServ sets mode: +o ironfoot | 11:38 | |
| *** kailueke[m] has joined #buildstream | 11:39 | |
| *** ChanServ sets mode: +o tristan | 11:41 | |
| tristan | And we're back !!! | 11:41 |
| tristan | Hmmmm, I have a sneaking suspicion that our tox testing stuff does not preemptively assert what version of buildbox is installed | 11:42 |
| tristan | I think the last merge requires a new one, as I'm seeing local test failures which don't happen in CI | 11:42 |
| *** jjardon_ is now known as jjardon | 11:44 | |
| *** ChanServ sets mode: +o jjardon | 11:45 | |
| tristan | Hmmm, how do I run this test... I'm trying: | 11:49 |
| tristan | tox -e py37-plugins -- src/buildstream/testing/_sourcetests/track.py::test_track_include_junction[ostree-project.refs] | 11:49 |
| tristan | I get: pytest: error: unrecognized arguments: --plugins | 11:49 |
| *** DineshBhattarai[m] has joined #buildstream | 11:58 | |
| *** albfan[m] has quit IRC | 12:07 | |
| *** Trevio[m] has quit IRC | 12:07 | |
| *** doras has quit IRC | 12:07 | |
| *** awacheux[m] has quit IRC | 12:07 | |
| *** kailueke[m] has quit IRC | 12:07 | |
| *** walterve[m][m] has quit IRC | 12:08 | |
| *** reuben640[m] has quit IRC | 12:08 | |
| *** DineshBhattarai[m] has quit IRC | 12:08 | |
| *** abderrahim[m] has joined #buildstream | 12:09 | |
| *** walterve[m][m] has joined #buildstream | 12:32 | |
| *** kailueke[m] has joined #buildstream | 12:32 | |
| *** reuben640[m] has joined #buildstream | 12:32 | |
| *** mattiasb has joined #buildstream | 12:32 | |
| *** Trevio[m] has joined #buildstream | 12:32 | |
| *** m_22[m] has joined #buildstream | 12:32 | |
| *** dbuch[m] has joined #buildstream | 12:32 | |
| *** tchaik[m] has joined #buildstream | 12:32 | |
| *** jjardon[m] has joined #buildstream | 12:32 | |
| *** dylan-m has joined #buildstream | 12:32 | |
| *** DineshBhattarai[m] has joined #buildstream | 12:32 | |
| *** tlater[m] has joined #buildstream | 12:32 | |
| *** segfault1[m] has joined #buildstream | 12:32 | |
| *** SamThursfield[m] has joined #buildstream | 12:32 | |
| *** connorshea[m] has joined #buildstream | 12:32 | |
| *** Demos[m] has joined #buildstream | 12:32 | |
| *** krichter[m] has joined #buildstream | 12:32 | |
| *** asingh_[m] has joined #buildstream | 12:32 | |
| *** albfan[m] has joined #buildstream | 12:32 | |
| *** theawless[m] has joined #buildstream | 12:32 | |
| *** doras has joined #buildstream | 12:32 | |
| *** awacheux[m] has joined #buildstream | 12:32 | |
| *** pro[m] has joined #buildstream | 12:32 | |
| *** skullone[m] has joined #buildstream | 12:32 | |
| *** cgm[m] has joined #buildstream | 12:32 | |
| tristan | gah, rabbit holes can be deep | 12:44 |
| tristan | So to address https://gitlab.com/BuildStream/buildstream/-/merge_requests/1935#note_349965798, I need to keep track of all loaded projects so I can NULL out their element/source factories, as our data model doesn't naturally fall out of scope (unfortunately, but that's a bigger problem to address) | 12:46 |
| tristan | But the artifact tests use a sneaky internal Project() object, which of course gets killed when cli.run() completes due to them all getting cleaned up | 12:47 |
| tristan | So, maybe just revert to the old code path, live with an additional table of PluginSource objects and document why this is | 12:48 |
| * tristan could alternatively *try* to turn that test into an end-to-end test but, that looks a bit too onerous | 12:51 | |
| tristan | benschubert, can you confirm if you are okay with the new version, with my 2 comments starting with: https://gitlab.com/BuildStream/buildstream/-/merge_requests/1935#note_349965798 ? | 13:06 |
| benschubert | tristan: answered, thanks! | 13:08 |
| *** thinkl33t has quit IRC | 13:10 | |
| tristan | Yeah, we wouldnt need to refactor the test if projects naturally fell out of scope | 13:15 |
| tristan | I could potentially change it so that the loader keeps track of the loaded projects instead, thus leaving the test's project alone | 13:15 |
| tristan | but it's all a mess | 13:15 |
| tristan | Let's leave it this way I guess | 13:15 |
| tristan | Honestly I think both are desirable: Data model properly falling out of scope, tests never using internals | 13:16 |
| * tristan thinks there was agreement in the past that we still wanted some "unit tests" which test internals, though | 13:17 | |
| benschubert | yep true. I agree with the falling out of scope part, I remember at least 20Go of data we managed to shove off like that :D | 13:19 |
| benschubert | though that would happen around the end so no change of such gains | 13:19 |
| *** tristan has quit IRC | 13:20 | |
| juergbi | benschubert: I've finally added a regression test to https://gitlab.com/BuildStream/buildstream/-/merge_requests/1926 (dynamic build plan) | 13:37 |
| *** tristan has joined #buildstream | 13:37 | |
| benschubert | juergbi: cheers, thanks! | 13:39 |
| benschubert | Is someone familiar with the 'public' data (the dynamic part specifically): do we store it in the element key? Do we do this with resolved variables or without? | 13:40 |
| juergbi | benschubert: yes, it's used in cache key calculation | 13:44 |
| juergbi | I would expect variables to be resolved before that | 13:44 |
| juergbi | otherwise variable changes that only affect public data wouldn't trigger cache key changes | 13:45 |
| juergbi | ah, the dynamic part, hm | 13:46 |
| benschubert | juergbi: they are not (or I broke them). and I am not sure: integration commands, for example, do we need to resolve before? that would seem weird to me? or am I missing something? | 13:46 |
| juergbi | why would resolving integration commands be weird? | 13:46 |
| benschubert | basically #1310 is due to integration commands not having their variables resolved | 13:47 |
| benschubert | (ahead of time) | 13:47 |
| benschubert | because I'm not sure we should break cache keys if we change how the element is layed out? That would break the children ones but why the first one? (I'm happy if it does, will just change where I fix the bug) | 13:48 |
| juergbi | I'm not quite sure I understand what you mean. however, public data is stored in the artifact. whenever it changes, the cache key needs to change as well | 13:51 |
| benschubert | ok, then I'll make sure we do expand the variables before storing it (which is not the case currently, or I might have broken it with the change there) | 13:51 |
| juergbi | maybe verify with tristan | 13:52 |
| benschubert | do you remember by any change where we set it? | 13:52 |
| benschubert | tristan: ^ ? :D | 13:52 |
| juergbi | see __public and __dynamic_public in Element | 13:52 |
| benschubert | mmh imght have misunderstoo dthem | 13:53 |
| benschubert | thought __dynamic_public was only loaded from artifacts | 13:53 |
| benschubert | I'll have a second look, thanks! | 13:53 |
| juergbi | if you haven't already, maybe write a regression test first and verify which commit broke it | 13:53 |
| *** ChanServ sets mode: +o tristan | 13:54 | |
| tristan | Not clear on the question | 13:54 |
| benschubert | juergbi: good point | 13:54 |
| tristan | We do consider it in the cache key, we used to be more selective about this | 13:54 |
| tristan | We stopped being selective and started always considering it at the moment that public data became mutable by elements | 13:55 |
| benschubert | tristan: is public data (and the dynamic one) to be stored in the artifact and cache key as resolved or unresolved | 13:55 |
| juergbi | checking again, we only use static/user-specified public data for cache key calculation | 13:55 |
| tristan | Right that is a bit more tricky, I'm not entirely clear, however I suspect it does not make a difference | 13:55 |
| juergbi | I assume for dynamic public data it's the plugin's unique_key responsibility | 13:55 |
| tristan | Aha | 13:55 |
| tristan | juergbi, I think the assumption is that considering user specified public data makes the input entirely deterministic | 13:56 |
| juergbi | yes, it makes sense | 13:57 |
| tristan | causing any side effects of propagated public data to be deterministic as well | 13:57 |
| juergbi | wondering about the variable resolution aspect | 13:57 |
| tristan | And we still only consider element configuration (at the Plugin.unique_key() level) | 13:57 |
| juergbi | maybe we actually can resolve it after loading public data from the artifact | 13:57 |
| benschubert | juergbi: that is a simple one line change that seems to pass tests indeed | 13:57 |
| tristan | I'm not sure, I have a feeling we must resolve it if we don't already, and consider the resolved input in the cache key | 13:58 |
| tristan | We explicitly do _not_ consider variables in cache keys | 13:58 |
| tristan | To reduce false cache misses | 13:58 |
| juergbi | yes | 13:58 |
| juergbi | so we definitely need to consider the expanded integration commands somewhere in the cache key | 13:58 |
| juergbi | question is whether this should be done in the element it's defined in or where it's used | 13:59 |
| juergbi | the former is probably easier to get it right (expanding the variables early) | 13:59 |
| juergbi | especially considering we implicitly expand variables in all other places now | 13:59 |
| juergbi | however, it might not be optimal with regards to minimal rebuilds | 14:00 |
| tristan | Agree that that would be easier to get right, yes | 14:00 |
| juergbi | not sure how feasible it is to resolve variables in public data late | 14:00 |
| juergbi | I also don't know whether the rebuild aspect makes a big difference in practice with how variables are actually used | 14:01 |
| tristan | When asking the question "Is an element different because it's public data changes when given different variables", I'm tempted to say yes | 14:01 |
| tristan | As the public data is input for reverse dependencies | 14:01 |
| juergbi | yes, having reverse dependencies consider it in their cache keys seems very odd | 14:02 |
| juergbi | and maybe public data is extracted from artifacts and used outside BuildStream itself | 14:02 |
| juergbi | that wouldn't be possible if the variables weren't already resolved | 14:02 |
| tristan | Well, it could indeed be more useful if people were to do that | 14:03 |
| tristan | I'm not sure it makes sense to open up that surface | 14:03 |
| tristan | I.e. if people wanted public data, it could trivially be exported into an artifact by a plugin | 14:03 |
| benschubert | > https://gitlab.com/BuildStream/buildstream/-/blob/master/src/buildstream/element.py#L812 | 14:03 |
| benschubert | agreed, so 'just' resolving `data` variables on that line and we should be good right? (obviously, adding tests :D ) | 14:03 |
| tristan | I'm not sure, I think we were erring on the side of considering the expanded public data in cache keys | 14:04 |
| tristan | Element.set_public_data() as I understand is only called when an element mutates it | 14:05 |
| benschubert | tristan: that's how an element can set public data AFAIK | 14:05 |
| juergbi | yes but I wouldn't resolve it there at all | 14:05 |
| benschubert | ah ok | 14:05 |
| juergbi | strings from the plugins should be used as is, imo | 14:05 |
| benschubert | misunderstood then | 14:05 |
| juergbi | we should resolve static public data early on | 14:05 |
| tristan | benschubert, Right, I thought the question is about expanding the user provided public data early and considering that in the cache key | 14:06 |
| tristan | Exactly | 14:06 |
| tristan | in fact, I don't see how the Element could dynamically set data with variables unresolved | 14:06 |
| tristan | so that part is already output | 14:06 |
| benschubert | juergbi: I might be missing something, but I though that `set_public_data` was the only way for an element to set some public data? | 14:06 |
| benschubert | do we have another API to do it? | 14:07 |
| benschubert | I meant more specifically this line: https://gitlab.com/BuildStream/buildstream/-/blob/master/src/buildstream/element.py#L828 | 14:07 |
| tristan | https://docs.buildstream.build/master/arch_data_model.html#element-data-structure | 14:07 |
| benschubert | that's the static public data | 14:08 |
| tristan | Yes | 14:08 |
| benschubert | and this one is already expanded | 14:08 |
| benschubert | the problem is with the dynamic one | 14:08 |
| tristan | I thought we were already talking about that | 14:08 |
| juergbi | but integration-commands are typically static | 14:08 |
| tristan | benschubert, was it expanded before your changes or not ? | 14:08 |
| juergbi | I thought it was an integration-commands issue | 14:08 |
| tristan | In any case, the set_public_data() I think is not an issue, there is no way to reasonably pass it unexpanded variables afaics | 14:09 |
| juergbi | I agree | 14:10 |
| juergbi | if an element wants to set dynamic public data using some variables, it should be the element's responsibility to use get_variable() and hand a resolved value to set_public_data() | 14:11 |
| benschubert | tristan: I need to check with before, but it's hard to verify since we were anyways expanding it at command time | 14:11 |
| benschubert | > if an element wants to set dynamic public data using some variables, it should be the element's responsibility to use get_variable() and hand a resolved value to set_public_data() | 14:11 |
| benschubert | I disagree, this is very error prone and will move us back to a state where variables are not resolved equally | 14:11 |
| tristan | benschubert, I'm interested in (A) Do we consider the public data expanded as a whole in cache keys ? (B) Do we only consider expanded integration commands ? | 14:11 |
| juergbi | benschubert: I think it would be a fairly special case to use variables at all for dynamic plugin data | 14:12 |
| tristan | benschubert, I don't know where that quote is coming from, maybe some docs somewhere | 14:12 |
| benschubert | A) depends if you are speaking about the static and the dynamic one | 14:12 |
| tristan | static | 14:12 |
| benschubert | static is expanded always | 14:13 |
| juergbi | the correct change, imo, is: generalize Element.__expand_splits() to expand variables in the complete public data instead of just splits | 14:13 |
| benschubert | If I print https://gitlab.com/BuildStream/buildstream/-/blob/master/src/buildstream/element.py#L299 , it is always expanded | 14:13 |
| juergbi | only splits, right? | 14:13 |
| benschubert | juergbi: ah! that seems sensible let me try that | 14:13 |
| benschubert | gah that seems right indeed | 14:13 |
| tristan | juergbi, And with that change, does the expanded static data become input for reverse dependencies ? if so, it must as a whole be considered in the key I think | 14:14 |
| juergbi | yes, __public is already considered in cache key calculation | 14:14 |
| juergbi | the issue is that it was only partially expanded | 14:14 |
| juergbi | switching to self.__variables.expand(unexpanded_public) should fix that | 14:14 |
| tristan | Okay this is making good sense | 14:14 |
| benschubert | juergbi: this change broke all my cache keys, seems it's the good one :) I'll need time to validate | 14:17 |
| benschubert | thanks both | 14:17 |
| benschubert | I didn't realize we were expanding only the splits | 14:17 |
| benschubert | well, this will end up simpler and cleaner | 14:17 |
| juergbi | yes, let's hope expanding variables in public data early will not turn out to be a bad thing | 14:20 |
| benschubert | I'll run the test suite as soon as I validated the fsdk case | 14:21 |
| juergbi | theoretically, it would also be possible to have separate __expanded_public that is returned by get_public_data() | 14:21 |
| juergbi | but not stored | 14:21 |
| juergbi | that would match more closely the previous behavior | 14:21 |
| juergbi | however, if the simple approach without special casing splits is sufficient and doesn't significantly hurt rebuilds, even better | 14:22 |
| juergbi | benbrown: the WSL runner seems to be offline again | 14:35 |
| juergbi | can I please get an approval for https://gitlab.com/BuildStream/website/-/merge_requests/140 ? | 15:10 |
| juergbi | ta traveltissues | 15:17 |
| juergbi | oh, actually, noticed a typo in the URL :-/ | 15:17 |
| juergbi | fixed | 15:18 |
| *** tpollard has quit IRC | 15:20 | |
| *** tpollard has joined #buildstream | 15:20 | |
| tpollard | So if I give a project a default target, I can junction it into another project and `bst build $junction.bst` and resolve a build down to a specified element. | 16:06 |
| tpollard | Would it be plausible to expect the same to happen if depending on a junction as a build dependency | 16:07 |
| tpollard | it seemingly doesn't work, but I was wondering it that could be expected behaviour | 16:07 |
| tpollard | (it currently will say as expected that you can't depend on junctions) | 16:08 |
| tpollard | https://gitlab.com/celduin/bsps/example-app/-/jobs/569617114 | 16:11 |
| tpollard | juergbi? ^ | 16:24 |
| WSalmon | hi tpollard | 16:25 |
| *** tpollard has quit IRC | 16:59 | |
| *** santi has quit IRC | 17:28 | |
| *** santi has joined #buildstream | 17:29 | |
| *** santi has quit IRC | 17:34 | |
| *** benbrown has quit IRC | 17:36 | |
| *** benbrown has joined #buildstream | 17:36 | |
| benbrown | juergbi: Not sure if my response came through earlier (SASL failed on a previous reconnect) | 17:37 |
| benbrown | I gave the WSL runner a kick | 17:37 |
| juergbi | benbrown: ta, seems to be working again | 19:41 |
| *** benschubert has quit IRC | 21:33 | |
| *** phildawson_ has quit IRC | 21:45 | |
| *** narispo has joined #buildstream | 21:51 | |
| *** cphang has quit IRC | 21:53 | |
| *** jude has quit IRC | 22:10 | |
| *** Gabriella8863 has joined #buildstream | 22:38 | |
| *** Gabriella8863 has quit IRC | 22:39 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!