| *** tristan_ has joined #buildstream | 00:27 | |
| *** Prince781 has quit IRC | 00:46 | |
| gitlab-br-bot | buildstream: issue #250 ("Typo in warning message") changed state ("opened") https://gitlab.com/BuildStream/buildstream/issues/250 | 00:49 |
|---|---|---|
| *** tristan has quit IRC | 01:33 | |
| *** mcatanzaro has quit IRC | 03:17 | |
| *** Prince781 has joined #buildstream | 04:14 | |
| gitlab-br-bot | buildstream: issue #250 ("Typo in warning message") changed state ("closed") https://gitlab.com/BuildStream/buildstream/issues/250 | 04:50 |
| gitlab-br-bot | buildstream: merge request (doc_staging_dir->master: Improve source plugin staging dir doc) #269 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/269 | 04:56 |
| gitlab-br-bot | buildstream: merge request (doc_staging_dir->master: Improve source plugin staging dir doc) #269 changed state ("merged"): https://gitlab.com/BuildStream/buildstream/merge_requests/269 | 05:16 |
| gitlab-br-bot | buildstream: issue #182 ("Closing non-existing workspace") changed state ("closed") https://gitlab.com/BuildStream/buildstream/issues/182 | 05:19 |
| gitlab-br-bot | buildstream: merge request (issue-182_Closing_non-existing_workspace->master: Fix for issue 182 Closing non-existing workspace) #227 changed state ("closed"): https://gitlab.com/BuildStream/buildstream/merge_requests/227 | 05:20 |
| gitlab-br-bot | buildstream: issue #181 ("bst build --track-except option can be specified without --track or --track-all") changed state ("closed") https://gitlab.com/BuildStream/buildstream/issues/181 | 05:29 |
| gitlab-br-bot | buildstream: merge request (issue-181_bst_build_--track-except->master: Fix for issue #181) #228 changed state ("closed"): https://gitlab.com/BuildStream/buildstream/merge_requests/228 | 05:29 |
| gitlab-br-bot | buildstream: merge request (sam/integration-tests-alpine->master: Use a custom base sysroot for the integration tests) #243 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/243 | 05:42 |
| *** Prince781 has quit IRC | 08:04 | |
| *** toscalix has joined #buildstream | 08:37 | |
| *** aday has joined #buildstream | 08:42 | |
| *** tristan has joined #buildstream | 08:45 | |
| *** valentind has joined #buildstream | 09:14 | |
| juergbi | hi tristan, i'd appreciate your input on a couple of points in !259 (configurable git submodule checkout) | 09:29 |
| juergbi | 1) https://gitlab.com/BuildStream/buildstream/merge_requests/259#note_58637760 | 09:29 |
| juergbi | what key takes priority, i'd be in favor of changing it (i.e., let the most specific config win) | 09:29 |
| juergbi | 2) https://gitlab.com/BuildStream/buildstream/merge_requests/259#note_58638624 | 09:29 |
| juergbi | shall we try and minimize cache key changes or we don't care that much for master? | 09:29 |
| *** jonathanmaw has joined #buildstream | 09:35 | |
| juergbi | ta | 09:43 |
| tristan | juergbi, comments up | 09:43 |
| tristan | I should add docs to http://buildstream.gitlab.io/buildstream/buildstream.plugin.html#buildstream.plugin.Plugin.get_unique_key explaining about this | 09:44 |
| juergbi | i'd like to avoid combining global 'ignore-submodules' with individual 'checkout' as that can be quite confusing | 09:44 |
| tristan | I thought I did at one point | 09:44 |
| tristan | juergbi, oh yeah I agree; it's either checkout or ignore all the way | 09:44 |
| juergbi | we could use ignore in both cases but i generally try to avoid double negative (ignore: false) | 09:44 |
| tristan | meh, that depends on how you look at it I guess - honestly I dont mind either way | 09:46 |
| tristan | I suggested that because I thought it might help reconcile docs and code | 09:46 |
| juergbi | i could definitely also live with 'ignore' as long as it's used all the way | 09:47 |
| tristan | combination of both is certainly horrible yes | 09:48 |
| juergbi | i'd keep it as checkout but leave that up to jonathanmaw, if you don't mind much either | 09:48 |
| tristan | I dont really mind, but we should not break cache keys for the sake of added config | 09:48 |
| juergbi | yes, i was also tending in that direction but wasn't sure whether we had a policy on that for master | 09:49 |
| tristan | if it's "checkout" then the cache key remains unaffected by a value of True | 09:49 |
| jonathanmaw | juergbi, tristan: okay, iiuc, I need to change it in two ways: | 09:51 |
| jonathanmaw | 1) submodule-level "checkout" config can override source-level (or project-level) config on whether to checkout | 09:51 |
| jonathanmaw | 2) these changes shouldn't change the cache keys | 09:51 |
| juergbi | correct | 09:51 |
| tristan | ohhhhh I didnt look at the whole patch | 09:52 |
| tristan | Why is there a project level config of checkout ? | 09:52 |
| juergbi | tristan: it's the generic project.conf configuration of sources, like we already have for elements | 09:52 |
| tristan | This is supposed to be done in the same manner we have overrides for element types | 09:52 |
| juergbi | it is | 09:52 |
| tristan | Ah, awesome | 09:52 |
| tristan | So that doesnt every come into play | 09:52 |
| tristan | i.e. it's not a value to be considered anywhere in code | 09:53 |
| tristan | (that's how I read jonathanmaw) | 09:53 |
| tristan | s/every/ever | 09:53 |
| juergbi | yes, it's handled separately, doesn't need to be taken into account for this change | 09:53 |
| jonathanmaw | AIUI the project overrides will be completely transparent to the sources, yeah | 09:53 |
| nexus | \o/ | 09:53 |
| tristan | exactly yeah :) | 09:53 |
| tristan | so it's all computed in _yaml anyway before the source ever sees it | 09:54 |
| tristan | jonathanmaw, one more thing | 09:56 |
| tristan | jonathanmaw, doc/source/formatintro.rst should be updated for the section describing yaml config priorities | 09:57 |
| tristan | i.e. here: http://buildstream.gitlab.io/buildstream/formatintro.html#composition | 09:57 |
| jonathanmaw | okie doke | 09:57 |
| tristan | I suppose Element Defaults needs to be changed, those are at the same priority | 09:58 |
| tristan | ohhh wait a sec | 09:58 |
| tristan | jonathanmaw, ah right yeah, that would be priority (4) in that list | 09:59 |
| tristan | it says you have another opportunity to override things on a per element basis | 09:59 |
| tristan | this is already wrong, it's per element-type, but should also be amended to speak of source overrides too | 09:59 |
| *** ssam2 has joined #buildstream | 10:08 | |
| *** dominic has joined #buildstream | 10:12 | |
| *** valentind has quit IRC | 10:29 | |
| *** jennis has quit IRC | 10:40 | |
| skullman | I'm taking a look at this ruamel integer underscore serialisation issue. | 11:27 |
| skullman | Current progress is I've found the code where it gets done | 11:27 |
| gitlab-br-bot | buildstream: issue #251 ("CI tests not working in other repositories") changed state ("opened") https://gitlab.com/BuildStream/buildstream/issues/251 | 11:28 |
| juergbi | skullman: you're working together with nexus on this? | 11:29 |
| nexus | juergbi: you just missed him, and kindof | 11:31 |
| juergbi | ok, just want to make sure you're coordinating | 11:32 |
| nexus | what is the best source of information about why a test failed, what result it got and what it expected? | 11:32 |
| tristan | The code in the test case | 11:33 |
| nexus | fair enough | 11:34 |
| paulsherwood | is there a canonical link to the beaver logo? | 11:39 |
| paulsherwood | and any info about who created it, licence etc? | 11:39 |
| ssam2 | https://www.dreamstime.com/royalty-free-stock-images-vector-cartoon-beaver-plumber-wrench-image16167719 | 11:39 |
| ssam2 | can also be downloaded from https://www.dreamstime.com/royalty-free-stock-images-vector-cartoon-beaver-plumber-wrench-image16167719 (although don't consider that 'canonical') | 11:40 |
| ssam2 | i paid them $7 so it's ours to use :-) | 11:40 |
| paulsherwood | oh, great. which licence applies, then? | 11:41 |
| ssam2 | good question | 11:42 |
| paulsherwood | :) | 11:42 |
| paulsherwood | maybe we should add the logo to the buildstream documentation, make that the canonical location for bst | 11:43 |
| ssam2 | i think it's more a mascot than a logo | 11:43 |
| ssam2 | but we could put it in there, yeah | 11:43 |
| * paulsherwood is thinkgin of tweeting about it | 11:44 | |
| ssam2 | i'll put some info on the wiki about the license etc. at least | 11:44 |
| ssam2 | although I don't know that the website is going to give us a useful one-line description of it... | 11:44 |
| ssam2 | must be something i can link to though | 11:44 |
| ssam2 | https://www.dreamstime.com/terms.php#royaltyfree | 11:44 |
| paulsherwood | ack | 11:45 |
| tristan | I think it's more of a mascot than a logo too, but he's a cute beaver and it doesnt hurt to put him somewhere visible :) | 11:46 |
| nexus | he is a cute beaver | 11:47 |
| ssam2 | i created https://wiki.gnome.org/Projects/BuildStream/Branding | 11:51 |
| ssam2 | in terms of 'canonical location for bst', we would do worse than to point people at https://buildstream.gitlab.io/ | 11:56 |
| ssam2 | currently that just redirects to the documentation | 11:56 |
| ssam2 | but it's actually a totally customizable website built from this repo https://gitlab.com/BuildStream/buildstream.gitlab.io | 11:57 |
| ssam2 | so it could later become a fully fledged website, or redirect to one | 11:57 |
| tristan | Yep, if anyone is up for designing a really nice spiffy and modern website, there is a place for it to live... | 11:57 |
| jmac | Can I specify an ordering for build dependencies? It looks like it's deepest tree first by default. | 12:00 |
| tristan | jmac, it's pre-sorted to be deepest first at load time yes | 12:03 |
| tristan | jmac, *why* ? | 12:03 |
| *** jennis has joined #buildstream | 12:04 | |
| jmac | Because I want to demonstrate an error which only happens with particular stage ordering | 12:04 |
| jmac | i.e. the order in which artifacts are combined to make the sandbox for a build affects whether sandbox creation breaks | 12:05 |
| tristan | Okay, so you need a combination of elements which cause this to happen | 12:07 |
| *** tm has joined #buildstream | 12:38 | |
| *** aday has quit IRC | 12:41 | |
| *** aday has joined #buildstream | 12:42 | |
| jmac | OK, solved that - it's not a dependency ordering problem | 12:46 |
| tristan | :) | 12:47 |
| jmac | There is however an implicit build dependency between anything with an integration command and the element that provides 'sh', which I'll add to the documentation to make it clear | 12:50 |
| *** aday has quit IRC | 12:53 | |
| gitlab-br-bot | buildstream: issue #252 ("Print summary of discovered cache keys at end of session.") changed state ("opened") https://gitlab.com/BuildStream/buildstream/issues/252 | 12:54 |
| paulsherwood | can we get buildstream.io ? | 12:58 |
| ssam2 | appears to be parked: http://www.buildstream.io/ | 13:00 |
| ssam2 | aka squatted | 13:00 |
| jmac | build.stream appears to be available, but expensive | 13:03 |
| juergbi | we need the .bst TLD ;) | 13:03 |
| tristan | jmac, yeah indeed; we may want to extend that in the future | 13:08 |
| tristan | jmac, one might want integration commands that are direct program invocations, or; for instance rpm allows one to specify the interpretor in which the postinst scriptlets should run | 13:09 |
| tristan | At least `bst shell -c -- /bin/foo` works without requiring a shell | 13:09 |
| *** mcatanzaro has joined #buildstream | 13:10 | |
| *** aday has joined #buildstream | 13:15 | |
| jmac | tristan: Yeah, I was just discussing that with ssam2 | 13:25 |
| adds68 | ssam2, this was the stuff he pinged over, he didn't have much time to write something up so just pinged the pain points: https://paste.codethink.co.uk/?3996 | 13:34 |
| ssam2 | ok, ta | 13:35 |
| persia | adds68: Could you put that somewhere where we could all see it? | 13:35 |
| adds68 | ssam2, i'm not sure how useful they are, just some feedback from a first user etc | 13:36 |
| ssam2 | makes sense | 13:37 |
| adds68 | persia, https://pastebin.com/id4XSte0 | 13:39 |
| persia | adds68: Thanks. | 13:40 |
| *** aday has quit IRC | 14:06 | |
| *** tristan has quit IRC | 14:08 | |
| *** aday has joined #buildstream | 14:11 | |
| *** tristan has joined #buildstream | 14:14 | |
| *** Benjamin has quit IRC | 14:19 | |
| skullman | ok, so ultimately the reason why ruamel's parser mishandles the numbers with underscores, is that it expects _ separators to only be used for separating thousands etc. and it calculates this based on the length of the last sequence of digits, and may have a leading or trailing underscore. | 14:35 |
| persia | Yes, but the key is that in practice a "track" value is a sequence of characters, whereas for many values that may be used, ruamel might choose to interpret as numbers (integer or floating point), which is confusing. | 14:39 |
| skullman | It should be possible to monkeypatch or extend RoundTripConstructor and RoundTripRepresenter to override its underscore handling and replace it with something that will records how long each sequence of underscores and digits is | 14:39 |
| skullman | persia: I'm aware of that, and ideally we'd say it has to be a string, but I'm told we can't do that because of stability guarantees, and most of the test suite failing if we try that doesn't help | 14:40 |
| persia | It must be a string. Changing the YAML structure to enforce that is something we cannot do. I'm a fan of the approach of special-casing the parsing to interpret "track" as a string. Others seem to think that implementations of that are unclean. | 14:41 |
| nexus | sadly that still fails the test cases | 14:42 |
| ssam2 | have you looked at modifying the test cases ? | 14:43 |
| nexus | ssam2: it's a runtime error | 14:43 |
| nexus | not a failed test | 14:43 |
| persia | nexus: The implementation causes them to fail, or the idea causes them to fail? | 14:43 |
| jmac | How so? Can I see the error? | 14:43 |
| persia | Last time I looked through failure logs, the problem seemed to be that the implementation broke ruamel because the underlying object changed between uses. | 14:44 |
| ssam2 | either the test fails or succeeds, i have no idea what "it's a runtime error" might mean | 14:44 |
| nexus | https://paste.codethink.co.uk/?3999 | 14:44 |
| persia | nexus: Could you put that somewhere we could all see it? | 14:44 |
| nexus | oops | 14:44 |
| nexus | sure 1 min | 14:45 |
| nexus | https://pastebin.com/Q3a5LrDv | 14:45 |
| ssam2 | what's the context of that ? | 14:45 |
| ssam2 | it looks like a test fails because there's an unhandled exception somewhere | 14:45 |
| nexus | test_build[zip] one of the tests | 14:45 |
| jmac | bizarre | 14:45 |
| ssam2 | the error message seems pretty clear to me | 14:46 |
| nexus | it errors inside the ruamel codebase | 14:46 |
| persia | Basically, ruamel gets unhappy if the underlying representation changes mid-processing. The parsing directives have to be sent *before* we read stuff, or we have to use a different set of parsing directives. | 14:46 |
| tristan | err | 14:46 |
| skullman | pyyaml had the facility to provide a path-based resolver, but it wasn't well documented, I'll see if I can remember how they work and if they still work in ruamel. | 14:47 |
| tristan | skullman, the bug with `bst track` is only one of the side effects of ruamel.yaml adhering to the YAML 1.1 spec; which states that underscores shalt be ignored | 14:47 |
| juergbi | nexus: can you also point to the commit/patch that you used to trigger this? | 14:47 |
| nexus | sure one minute | 14:48 |
| juergbi | i.e., is this with your original patch (parse ints as strings) or something else? | 14:48 |
| tristan | skullman, that said; what we ultimately want is to avoid this spec quite brutally, so that anything is loaded as only strings (or dicts or lists but you get the meaning) | 14:48 |
| gitlab-br-bot | buildstream: merge request (issue-166_yaml_removing_underscores->master: WIP: Issue 166 yaml removing underscores) #245 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/245 | 14:48 |
| tristan | skullman, consider another side effect is that many release tags or even branches have the form "1.2" for instance | 14:48 |
| nexus | https://gitlab.com/knownexus/buildstream/tree/issue-166_yaml_removing_underscores | 14:49 |
| persia | tristan: The problem with loading everything as strings is that we run into a problem with numjobs. We could special case numjobs, converting to an integer before using it, of course. | 14:49 |
| tristan | skullman, that means ruamel.yaml will load it as a double/floating point, and then we trust blindly that we dont get rounding errors somewhere and convert back to a string when saving. | 14:49 |
| persia | Actually, special-casing numjobs handling is probably more sensible. | 14:49 |
| juergbi | nexus: ah, here you're again dynamically changing the constructor | 14:49 |
| tristan | persia, you probably mean max-jobs | 14:49 |
| persia | tristan: Yes: my memory is unreliable :) | 14:49 |
| nexus | juergbi: yes | 14:49 |
| juergbi | nexus: always parsing as strings should avoid this exception | 14:49 |
| tristan | persia, and that is an internal issue; nobody told me exactly what that was; but it should be a non-issue really | 14:50 |
| nexus | it doesnt | 14:50 |
| juergbi | persia: max-jobs is used as variable, i.e., always as string | 14:50 |
| tristan | persia, it's possible something is accessing that max-jobs without going through the expected _yaml.node_get() code path which is supposed to do the conversion to the desired type | 14:50 |
| persia | juergbi: In an earlier revision of nexus' patch, everything was parsed as string, and there was an exception about max-jobs, as it wanted a number. | 14:50 |
| juergbi | persia: i assume the issue is that we initialize it as int in the code in a couple of places | 14:50 |
| tristan | so it gets a string there, without asking _yaml.node_get() to produce a number | 14:50 |
| juergbi | but that could easily be changed | 14:50 |
| persia | juergbi: Precisely. | 14:50 |
| persia | Right, so I guess the best way is to go back to the original ruamel fix (parse everything as strings), and then fix max-jobs handling internally. | 14:51 |
| tristan | it's also possible that _yaml.node_get() needs to be fixed or touched up to do the right thing once we get the desired unconverted raw strings from ruamel.yaml | 14:51 |
| tristan | if anything is accessing nodes directly, that is anyway a bug | 14:51 |
| juergbi | yes, we probably have to convert to 'expected_type' in node.get | 14:52 |
| tristan | indeed max-jobs is a string anyway | 14:52 |
| tristan | _yaml.node_get() does have a type coercion as I recall, but it may need fixing with this change | 14:53 |
| persia | nexus: Do you have a link to the previous merge request (interpret everything as strings)? | 14:54 |
| * tristan recalls a very cryptic one liner that tells yaml something very mumbo-jumbo-ish :) | 14:55 | |
| nexus | persia: yes | 14:55 |
| persia | tristan: That tells ruamel something mumbo-jumboish, yes. | 14:55 |
| nexus | RoundTripConstructor.add_constructor(u'tag:yaml.org,2002:int', RoundTripConstructor.construct_yaml_str) | 14:55 |
| nexus | ^ | 14:55 |
| tristan | haha yeah that's it: u'tag:yaml.org,2002:int' | 14:56 |
| persia | nexus: That's not the link to the old merge request. Could you share that? The CI report is also interesting. | 14:57 |
| juergbi | https://gitlab.com/BuildStream/buildstream/commit/b9d878fc4ac479e0db61bd7d42f274c976176d23 | 14:57 |
| nexus | ^ | 14:57 |
| juergbi | https://gitlab.com/knownexus/buildstream/pipelines/16334340 | 14:57 |
| tristan | oh yeah, please fix the whitespace error on line 26 | 14:58 |
| nexus | that gives the same error | 14:58 |
| nexus | the whole "add_constructor" thing is what causes the "RuntimeError: OrderedDict mutated during iteration" | 14:58 |
| juergbi | i just ran this locally and the only test failure i got was | 14:58 |
| juergbi | tests/context/context.py::test_context_load_invalid_type FAILED | 14:58 |
| nexus | and i will before submitting tristan :) | 14:58 |
| persia | nexus: No. adjusting it mid-parse is what gives that. There was a CI run that only failed on max-jobs with the parser adjustment, wasn't there? | 14:59 |
| nexus | i just did a quick patch on my version, so i may have missed somethingh | 14:59 |
| juergbi | the pipeline link i \posted was wrong | 14:59 |
| juergbi | nexus: make sure you rebase on top of latest master, the MR is quite a bit behind | 15:00 |
| persia | https://gitlab.com/knownexus/buildstream/-/jobs/48575837 has "Error loading pipeline: manual-noparallel-test.bst [line 5 column 2]: Expected 'int' type for configuration 'max-jobs', instead received 'str'", which is the max-jobs issue (which needs to be handled differently) | 15:01 |
| juergbi | right, integration tests are still in progress locally | 15:03 |
| juergbi | ok, makes sense. that's because it composites the default (which is an int in Python code) with the custom max-jobs | 15:04 |
| juergbi | and the composition bails out if the types don't match | 15:04 |
| juergbi | so changing max-jobs init to str should fix it | 15:05 |
| nexus | ok, got my version back, lets see the damage | 15:06 |
| nexus | juergbi: where's that? | 15:07 |
| tristan | juergbi, ahhh indeed, this is nothing to do with _yaml.node_get(), this is dictionary composition; where types are strict | 15:08 |
| juergbi | nexus: https://pastebin.com/MxYM2iaU | 15:08 |
| tristan | _yaml.py:736 | 15:08 |
| juergbi | with this all tests pass except the yaml invalid type test but i think we should just remove that | 15:08 |
| juergbi | tristan: yes, looks all pretty good now afaict with that patch | 15:09 |
| persia | juergbi: So those two changes (for max-jobs), plus the blanked ruamel fix does the job? | 15:09 |
| juergbi | at least it passes the test | 15:09 |
| tristan | what is the yaml invalid type test ? | 15:09 |
| juergbi | we may need to check whether --track-save now outputs quotes where it didn't before. if we care about this | 15:09 |
| * tristan is looking in tests/yaml/yaml.py | 15:09 | |
| juergbi | tristan: it tests that some-string-key: 3 is rejected | 15:10 |
| juergbi | but as we interpret all based on expected_type now, it won't fail, of course | 15:10 |
| persia | Adding a artificial --track-save test to replace the yaml-invalid-type test is probably sane. | 15:10 |
| juergbi | it will simply be interpreted as string "3" | 15:10 |
| * tristan doesnt see where it is | 15:10 | |
| persia | Because if we are telling ruamel to read everything as strings, there can't be an invalid type (as there is no real type detection) | 15:10 |
| tristan | persia, but there can | 15:11 |
| tristan | it seems that this test would be better suited the other way around | 15:11 |
| persia | tristan: How? If everything is a string, isn't everything a string? | 15:11 |
| tristan | persia, "five" is not a valid int, though | 15:11 |
| * persia reads the actual test case | 15:11 | |
| persia | tristan: yes, but we can't get integers from yaml anymore if we apply the mumbo-jumbo | 15:11 |
| tristan | persia, from what I understand, this is testing that _yaml.node_get() will abort with the proper errors when a value of an expected type cannot be loaded | 15:11 |
| tristan | persia, the core cares about the output of _yaml.node_get(), how that works underneath does not change the upper API contract | 15:12 |
| tristan | but I still dont see the test :-S | 15:12 |
| * tristan greps for "some-string-key" | 15:12 | |
| * tristan cant find any such test | 15:14 | |
| gitlab-br-bot | buildstream: merge request (issue-166_yaml_removing_underscores->master: WIP: Issue 166 yaml removing underscores) #245 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/245 | 15:14 |
| juergbi | tristan: https://gitlab.com/BuildStream/buildstream/blob/master/tests/context/data/invalidtype.yaml | 15:15 |
| persia | tristan: Test is tests/context/context.py:119 | 15:15 |
| tristan | context! | 15:15 |
| juergbi | sorry, it wasn't literal | 15:15 |
| juergbi | you were missing the context of the test ;) | 15:15 |
| persia | I really don't think that test adds value with RoundTripConstructor.add_constructor(u'tag:yaml.org,2002:int', RoundTripConstructor.construct_yaml_str) | 15:15 |
| tristan | juergbi, indeed | 15:16 |
| juergbi | the inverse suggested by tristan makes sense to me | 15:16 |
| juergbi | not sure whether the conversion currently bails out and if it does, whether the error message is sensible | 15:16 |
| tristan | juergbi, context helps... but yeah I think we set the scheduler fetchers to "twenty-two" or something | 15:16 |
| tristan | that would preserve the test | 15:16 |
| persia | Checking to make sure that it isn't an integer? Yes, that guards against someone removing the parser modification. | 15:16 |
| juergbi | persia: the idea is to test that a field that expects an int rejects non-ints | 15:17 |
| persia | Oh, setting something we plan to convert to an integer to a string? Yes, that is also a reasonable test. | 15:17 |
| juergbi | the test you just suggested makes indeed sense as well, though | 15:17 |
| juergbi | have 1_2_3 in a yaml file and make sure we can retrieve it as string with the underscores | 15:18 |
| persia | An interesting question for things we plan to use as integers, given that they are parsed as strings, are we testing the cast? | 15:18 |
| juergbi | tristan's test suggestion would test the cast | 15:18 |
| persia | I like the 1_2_3 test. That will help make sure the underlying bug that the suggested fix handles does not return. | 15:18 |
| tristan | persia, fairly much I believe, but this is one of them | 15:18 |
| tristan | we probably need an additional one for 1_2_3 and 1.2.3 | 15:19 |
| persia | Yes. Both _ and . should be tested. | 15:19 |
| tristan | to guard against regressions of _this_ fix | 15:19 |
| juergbi | yes, we haven't fixed the float case yet, although it should be identical | 15:19 |
| nexus | juergbi: what tests am i expecting to pass/fail rn? | 15:28 |
| persia | nexus: With the blanket roundtripconstructor and juergbi's max-jobs patches, you should have no failures | 15:29 |
| juergbi | tests/context/context.py::test_context_load_invalid_type FAILED | 15:30 |
| juergbi | this will fail, nothing else should fail | 15:30 |
| persia | Rather, you should have just the context.py invalidtype failure, which will need new adjustment to fix the cases immediately discussed. | 15:30 |
| * nexus starts reading backlog | 15:31 | |
| tristan | context.py test_context_load_invalid_type needs to change it's data, to reverse it so that we try to load an int from something that cannot be converted to int | 15:31 |
| tristan | the tests for 1_2_3 and 1.2.3 are separate things, though | 15:31 |
| persia | tristan: Do you think they belong as tests in context.py, or somewhere else? | 15:32 |
| tristan | They could go in many different places, maybe a parameterized test added to tests/yaml/yaml.py would be a good place | 15:32 |
| tristan | persia, well, context.py is testing that the Context object itself bails out when parsing something invalid (this still needs converting to proper `cli` frontend tests, also) | 15:33 |
| tristan | so it's a valid test there; I'm just saying there is no real need to cram these new tests into something "Context specific" | 15:33 |
| persia | Makes sense. Let's put the 1.2.3 and 1_2_3 tests in tests/yaml/yaml.py, so they are easier to find :) | 15:33 |
| tristan | yeah I think a single parameterized test which shoots a bunch of data at _yaml.node_get() would cover this well | 15:34 |
| nexus | yep tests/context/context.py:128: Failed | 15:35 |
| nexus | Did we decide on a fix for context? | 15:54 |
| persia | nexus: Yes. Change the invalid yaml to load something in an integer context with something that cannot be cast. Something like "max-jobs: five" | 15:57 |
| persia | Or setting the scheduler fetcher count to "twenty-two" | 16:00 |
| juergbi | that's actually a bad example as max-jobs is just a variable and processed as strings | 16:00 |
| nexus | ok, i'm likely going to need assistance from someone who's worked with the tests before | 16:00 |
| juergbi | fetchers/builders/pushers options should work for this test, though | 16:00 |
| *** Prince781 has joined #buildstream | 16:12 | |
| jmac | Is there such a thing as an optional dependency? I.e. "I can use this as part of the build if available, please include it if it does not cause a circular dependency" | 16:13 |
| juergbi | jmac: no, that would be tricky to resolve properly | 16:13 |
| jmac | Yes, it would :) | 16:14 |
| juergbi | as you may have to schedule build jobs for the same element twice in a single session | 16:14 |
| juergbi | right now you have to create two elements for the same source in that case | 16:14 |
| jmac | That's fine | 16:14 |
| juergbi | conditionals are supported but you can't include the same element twice (with different options) in a single session | 16:15 |
| juergbi | (unless you use a junction pointing to the same project but let's not go there) | 16:15 |
| gitlab-br-bot | buildstream: issue #232 ("fd leak when generating epiphany tarball") changed state ("opened") https://gitlab.com/BuildStream/buildstream/issues/232 | 16:19 |
| persia | if one needs two variants of a build, I think having two elements is cleaner, as they presumably serve different purposes. It would be nice that if tristan's idea about separating the refs from the .bst files was implemented, both elements could use the same index to get the ref. | 16:20 |
| jmac | My particular case is that I would like to use reprotest as part of the build process for all elements where reprotest can sensibly be available | 16:21 |
| tristan | jmac, this sounds somewhat unwieldy to me; you mean to say that randomizing the build environment in the way reprotest does; requires that you inject something as a dependency into the runtime itself ? | 16:26 |
| persia | Are there many cases where it cannot be sensibly available? My initial thought would be to create a base platform that included reprotest, and use that to build everything. | 16:26 |
| tristan | jmac, sounds like reprotest is dealing with the problem of self-hosting bound distros and it makes sense for them; but for BuildStream it means that people need to construct projects in special ways in order to test for reproducibility ? | 16:27 |
| persia | Unless I misunderstand, the goal is to determine whether an upstream source can be reproducibly built, despite adjustments X, Y, and Z. If we're testing whether BuildStream can reproduce a reproducible build, I would expect a different approach. | 16:27 |
| jmac | tristan: There are two ways of doing this | 16:27 |
| tristan | I'm sure there is a part of the story I'm missing here | 16:27 |
| jmac | You can either modify buildstream so as to vary the environment in the same way reprotest does, | 16:28 |
| jmac | or you can build reprotest as a buildstream element and then make it part of the build process of any later elements, requiring no change to buildstream | 16:28 |
| tristan | but requiring users to have some pretty in depth knowledge and manually do the reprotest themselves | 16:29 |
| jmac | No, not really? | 16:29 |
| tristan | which turns the project into a sort of proof of concept; where the deliverable is a blog post saying "Look, you can use BuildStream in this way too !" | 16:29 |
| tristan | Which, is not a bad thing really | 16:29 |
| tristan | Well, considering the case where you have a bunch of modules, you have a buildstream project which has some branches (production, testing, master, various work branches) | 16:30 |
| tristan | It means that if you want a reprotest mode, you have to satisfy reprotest's dependencies conditionally (cause you dont want to include reprotest in your firmware...) or perhaps have elements only build-depend on it | 16:31 |
| jmac | Yes, you only need it as a build dependency | 16:31 |
| tristan | Well, it's not a bad thing; but it certainly does not amount to a feature afaics | 16:31 |
| jmac | This is freedesktop-sdk's nano.bst modified to build with reprotest: https://paste.gnome.org/plukvj04i | 16:32 |
| jmac | The build will fail if not reproducible, but the produced artifact should be identical to the non-reprotest version | 16:33 |
| tristan | I'm not going to be a stickler for these details though, if we dont have a feature that people can use out of the box, it's not really bad | 16:33 |
| jmac | It's entirely possible to keep reprotest permanently on for all elements, but it will double the build time | 16:33 |
| tristan | Personally I dont feel like it's something I want to recommend, conditional statements should hopefully not be very many | 16:34 |
| jmac | persia: The question of whether BuildStream can build reproducibly is trivial to test; I posted to the list about it on 2018-02-01 | 16:35 |
| tristan | We already expect conditionals in some cases where modules need to be informed of target arch if they can't guess it themselves, or when you build multiple outputs | 16:35 |
| jmac | Indeed, I think at the moment all this will be is a set of instructions and example files for running reprotest inside BuildStream if you want to; it might generate some more feedback on how people want to do this. | 16:36 |
| tristan | I mean, people might want to do this, and it's a bit sad that they have to themselves go to the length of understanding reprotest and integrating it into their build with conditional statements to do it | 16:36 |
| jmac | Really, there's no understanding of reprotest required | 16:36 |
| persia | jmac: Yes, and I've pasted the link to the archives of your post in a number of fora: that's why I think the current problem is "test whether a given source can be built reproducibly", rather than "can buildstream build a reproducible build with bit-for-bit identity" | 16:36 |
| tristan | well, at least you have to understand it's dependencies right ? | 16:36 |
| tristan | And how to invoke it | 16:37 |
| tristan | persia, yes that's the topic we're on | 16:37 |
| jmac | I'll be providing examples for that | 16:37 |
| jmac | The question of what you do when something isn't reproducible is a lot harder to explain | 16:38 |
| persia | Fix it? | 16:38 |
| tristan | persia, we are currently setting up a model buildstream project which implements this on the project side; which will be a burden for any project author to implement themselves; if the randomization it does is trivial, it seems illogical to not provide an actual feature if it's really a desirable thing | 16:38 |
| jmac | persia: Yes. | 16:38 |
| persia | tristan: You mean, reimplement the reprotest variance injection as something that can be defined in project.conf? | 16:39 |
| tristan | persia, I'm not sure of the details of what it does; but BuildStream dictates already the build environment, if the build could be run with a --reprotest option, we could run the same builds many times with randomized environment vars and such trickery | 16:41 |
| persia | tristan: https://anonscm.debian.org/git/reproducible/reprotest.git/tree/ if you want a quick glance. | 16:41 |
| tristan | so project maintainers could theoretically get this quite for free, without having the tailor projects to use a special piece of software inside their runtime | 16:41 |
| jmac | tristan: That's entirely possible, but it requires extracting all the variation options out of reprotest and reimplementing them. I estimated it'd take a lot longer to do it that way. | 16:42 |
| jmac | The nature of the variations means the means of doing them is often quite "hacky" | 16:42 |
| persia | To me, it looks like reprotest needs coverage, pytest, and rstr, assuming we always use --no-diffoscope | 16:43 |
| tristan | I'm seeing it now yeah, pretty smart stuff, i.e. rational variations of TMPDIR etc | 16:43 |
| persia | Can we just import it, and use the existing code? | 16:43 |
| ssam2 | perhaps we could set up a separate project containing reprotest | 16:43 |
| persia | Adds to buildstream dependencies, but perhaps not that much | 16:44 |
| ssam2 | and recommend people import that with a junction in order to use it in their projects | 16:44 |
| tristan | which seems rather tricky to implement (but I'm not sure how reprotest could even implement that; doesnt it need to also ensure that TMPDIR points to a filesystem that implements mmap and is writable ? | 16:44 |
| tristan | ) | 16:44 |
| ssam2 | it would probably pull in a separate base, but as long as that is only a build-time dep, it wouldn't mess up the actual main project's output | 16:44 |
| persia | ssam2: Still requires custom build instructions | 16:44 |
| ssam2 | agreed | 16:44 |
| persia | ssam2: See https://paste.gnome.org/plukvj04i | 16:44 |
| ssam2 | but less confusion about having deps in the main project repo that are only there to satisfy reprotest | 16:44 |
| persia | My thought was that we would simply consider reprotest a dependency of BuildStream (and so accept transitive dependencies), to enable the feature. | 16:45 |
| tristan | ssam2, you need a reprotest in the runtime that you are actually building against, unless you can provide a stock one and run "chroot" as a part of the build, *under* reprotest somehow | 16:45 |
| persia | That limits reimplementation to _main_.py, mostly | 16:45 |
| tristan | persia, that thought sounds like the least likely of any scenario | 16:45 |
| ssam2 | hmm, yeah i guess layering one runtime on top of another isn't going to work | 16:45 |
| tristan | persia, I'm not sure you can import reprotest python and do something useful with that inside buildstream | 16:46 |
| tristan | well, I highly doubt it | 16:46 |
| persia | tristan: Why not? | 16:46 |
| tristan | persia, I'm assuming reprotest sets up it's build environment and sandbox etc | 16:46 |
| tristan | sounds like direct collision with buildstream codepaths; hey I could be wrong | 16:47 |
| tristan | but intuitively it sounds like the least likely | 16:47 |
| persia | tristan: Yes, but my thought was to not use those bits, and instead just use the libary bits that reprotest uses. | 16:47 |
| tristan | I dont know; maybe | 16:47 |
| *** ernestask has joined #buildstream | 16:48 | |
| persia | I feel fairly strongly that the reprotest upstream folk are more motivated than us to develop new sources of variation, etc. Having a common library of alterations that can affect the sandbox strikes me as useful, as otherwise there is a lot of copy&paste work. | 16:48 |
| jmac | That was my thought too; BuildStream does a lot to control the build environment and I thought that would clash with reprotest's attempts to do the same | 16:49 |
| tristan | persia, well, it's plausible that the reprotest folks will be amenable to making it possible if it's not | 16:49 |
| persia | tristan: They are friendly folk. | 16:49 |
| tristan | persia, seems more like a database of scenarios we'd want to share than a library | 16:50 |
| tristan | yeah I know, I hang out in #reproducible-builds | 16:50 |
| persia | tristan: Maybe, although as far as I can tell, that database currently only exists as python code. | 16:50 |
| tristan | yeah that's a detail; look, someone has to actually read the code and say how amenable the codebase is of this sharing before we can constructively continue down this avenue | 16:51 |
| tristan | currently jmac knows most afaict | 16:51 |
| tristan | jmac, how would you like to evaluate that road too ? i.e. see if we can reuse parts of reprotest directly, if it's structured in such a way that it might work ? | 16:52 |
| persia | Fair: I'm happy to pre-decide that the conclusion of the discussion should be "whichever is less work" in terms of reimplementation vs. reuse | 16:52 |
| tristan | I mean, my gut tells me it's purposed for it's own thing, also there is the avenue of collaboration | 16:52 |
| jmac | tristan: I'd like to do that. I may need to negotiate to get time to do it, though. | 16:53 |
| tristan | jmac, yeah, well persia, ltu and I have a meeting tomorrow, I think that persia might be able to make a call on whether it's worth looking into, considering what we're already doing | 16:54 |
| tristan | persia, you said you were under the impression that we were using reprotest as a dependency of BuildStream and implementing a feature in BuildStream with it... | 16:55 |
| tristan | persia, if that's what is mandated, then I suggest we budget the time hehe | 16:55 |
| persia | Did I? I thought I said I thought that made sense as a path to achieve the goal. | 16:55 |
| tristan | persia, I misread you then, "but my thought was ..." | 16:56 |
| tristan | depends on when "was" happened I guess :D | 16:56 |
| persia | Anyway, I think we've beaten the horse enough :) | 16:56 |
| tristan | yeah | 16:57 |
| tristan | I'm not personally running after the feature also, just raising a flag that this might not be what we're looking for | 16:57 |
| *** dominic has quit IRC | 16:58 | |
| skullman | :¬/ the ruamel path resolver code doesn't work with code known to work with pyyaml, and the pyyaml resolver gave precedence to implicit resolve rules over path based so wouldn't be useable to selectively choose which nodes should be integers anyway unless we were to start from the base parser | 17:04 |
| persia | In that case, the "workaround" of informing ruamel to parse everything as strings, and then doing the integer casts ourselves is probably actually the "fix", rather than just a "workaround". | 17:06 |
| skullman | that, or fix the integer parsing to preserve underscores | 17:11 |
| jmac | You still have the decimal point case | 17:12 |
| * tristan doesnt understand the desire to not just use the original approach, or has not yet understood that there is a problem with the original approach | 17:13 | |
| * juergbi is also not aware of an issue with that approach | 17:14 | |
| tristan | I mean; it's clearly breaking the YAML spec | 17:14 |
| juergbi | that would be the case no matter how we implement the fix | 17:14 |
| tristan | But that is known; we just cannot expect people to enter stuff like `track: "1.2"` | 17:14 |
| skullman | I'm not up with how buildstring loads yaml, but 1.2.3 would parse as '1.2.3' so stay as a string | 17:15 |
| tristan | exactly | 17:15 |
| tristan | skullman, 1.2 will load as a number, but this is dangerous | 17:15 |
| tristan | skullman, because floating point conversions from and to string format | 17:15 |
| skullman | I see your concern now. | 17:16 |
| persia | 1.2 isn't such an issue. Versions for things like tex get awkward. | 17:16 |
| persia | (where an additional digit of a trancendental number is added for each new version, in such a way that rounding rewinds history, if you are lucky) | 17:17 |
| tristan | in any case; the initial approach seems logical, load all normal values as strings nips any such problems at the bud | 17:17 |
| * persia does a bit of research, and is impressed with tex's average of one release each 4 years, as a statement of correctness of the original implementation | 17:20 | |
| *** valentind has joined #buildstream | 17:20 | |
| skullman | looking at the ruamel code, a 1.2 will round-trip serialisation to and from yaml correctly, but str(ScalarFloat) may produce something else entirely | 17:21 |
| juergbi | also need to support 1.20 (trailing zeroes) | 17:22 |
| persia | Yes. 1.20 != 1.2 only works with strings. | 17:22 |
| juergbi | which might be a good test case | 17:22 |
| skullman | >>> from ruamel import yaml | 17:27 |
| skullman | >>> yaml.round_trip_load('1.20') | 17:27 |
| skullman | 1.2 | 17:27 |
| skullman | >>> yaml.round_trip_dump(yaml.round_trip_load('1.20')) | 17:27 |
| skullman | '1.20\n...\n' | 17:27 |
| persia | Interesting. | 17:28 |
| jennis | When linting https://gitlab.com/BuildStream/buildstream/blob/master/buildstream/_yaml.py#L324 , the argument `indices=[]` is causing a "dangerous-default-value" error | 17:35 |
| jennis | There is an easy fix to this: `indices=None` as argument and within the function: `if indices is None: indices = []` | 17:35 |
| jennis | Now this isn't just a 'best practice' issue, I think it will actually effect the functionality of the code, see: https://stackoverflow.com/questions/26320899/why-is-the-empty-dictionary-a-dangerous-default-value-in-python/26320938 | 17:37 |
| juergbi | jennis: i don't see an actual issue in this particular case (we don't modify that empty list) | 17:37 |
| juergbi | however, we should still avoid it | 17:38 |
| juergbi | in this case it might even be sufficient to change the default value to None without even adding anything to the function body | 17:38 |
| juergbi | if i haven't missed anything | 17:38 |
| jennis | mhmm it's just line 332 that worries me | 17:39 |
| persia | The only issue is if someone called the function with "indices=4" in the arguments, it might not fail as soon without being type-guarded. | 17:39 |
| *** ssam2 has quit IRC | 17:40 | |
| jennis | because if you have `indices=None`, surely line 332, `if indices:` won't execute...? | 17:40 |
| *** Prince781 has quit IRC | 17:41 | |
| juergbi | jennis: empty list is considered False | 17:41 |
| juergbi | so it won't be executed in that case either | 17:41 |
| juergbi | and if indices is left at the default, we don't want to execute that block | 17:41 |
| jennis | is the argument basically saying, take the list indices and if there is no such list, take an empty list...? | 17:43 |
| juergbi | if the caller doesn't specify an argument for the `indices` parameter at all, the default value will be used, in this case [] | 17:44 |
| juergbi | (or None after the fix) | 17:44 |
| jennis | ahh ok, so I think the fix is better | 17:45 |
| jennis | but yes, because we're not actually making any changes to indices in the function, both will work...? | 17:46 |
| gitlab-br-bot | buildstream: merge request (juerg/ci->master: .gitlab-ci.yml: Support test execution in other repositories) #273 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/273 | 17:50 |
| juergbi | jennis: yes | 17:51 |
| *** jonathanmaw has quit IRC | 18:10 | |
| gitlab-br-bot | buildstream: issue #251 ("CI tests not working in other repositories") changed state ("closed") https://gitlab.com/BuildStream/buildstream/issues/251 | 18:17 |
| gitlab-br-bot | buildstream: issue #251 ("CI tests not working in other repositories") changed state ("closed") https://gitlab.com/BuildStream/buildstream/issues/251 | 18:17 |
| gitlab-br-bot | buildstream: merge request (juerg/ci->master: .gitlab-ci.yml: Support test execution in other repositories) #273 changed state ("merged"): https://gitlab.com/BuildStream/buildstream/merge_requests/273 | 18:17 |
| *** toscalix has quit IRC | 19:34 | |
| *** tm has quit IRC | 20:59 | |
| *** mcatanzaro has quit IRC | 21:22 | |
| *** ernestask has quit IRC | 21:37 | |
| *** aday has quit IRC | 22:21 | |
| *** valentind has quit IRC | 22:22 | |
| *** Prince781 has joined #buildstream | 23:22 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!