| *** mcatanzaro has joined #buildstream | 02:01 | |
| *** mcatanzaro has quit IRC | 02:10 | |
| *** tristan has joined #buildstream | 05:51 | |
| *** ChanServ sets mode: +o tristan | 05:56 | |
| juergbi | tristan: i'm just looking at the CI issue and see that the dist directory contains buildstream/integration-cache/sources/ostree/https___sdk_gnome_org_repo_ with tons of files | 07:13 |
|---|---|---|
| juergbi | already before the sdist | 07:13 |
| juergbi | so the tar -ztf dist/* fails because of * matching that extra directory | 07:13 |
| juergbi | any idea what's responsible for this? | 07:14 |
| juergbi | tlater: i suspect the change in your branch to (temporarily) add dist/buildstream/integration-cache/ as cache path is responsible for the CI issue | 07:26 |
| juergbi | i'll clear runner caches, let's see whether that helps | 07:26 |
| juergbi | yes, source_dist now succeeds again | 07:28 |
| *** valentind has joined #buildstream | 07:29 | |
| gitlab-br-bot | buildstream: merge request (juerg/recursive-pipelines->master: WIP: Junctions and subprojects) #160 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/160 | 07:38 |
| gitlab-br-bot | buildstream: merge request (juerg/recursive-pipelines->master: Junctions and subprojects) #160 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/160 | 07:39 |
| tlater | juergbi: Yes, that's definitely it - sorry, didn't realize that cache would be shared by all runners | 08:09 |
| * tlater still wonders how he can cache that dir but avoid its inclusion in the sdist | 08:11 | |
| tristan | Eeek | 08:12 |
| tristan | I see, yeah; dist is an artifact, not "the cache" :) | 08:13 |
| tristan | tlater, would be good to make sure your branch tip doesnt have that soon; as the next subsequent CI will reintroduce the breakage | 08:13 |
| tlater | Yep, I'll only enable caching when I actually merge | 08:14 |
| tristan | tlater, how's it coming along btw ? | 08:14 |
| tlater | Though this will probably break CI for branches that don't rebase | 08:14 |
| tristan | I'm excited to drop the old stuff | 08:14 |
| * tlater is still looking at the out of space issue | 08:14 | |
| tlater | Should hopefully be done today | 08:15 |
| tlater | ssam2 also had a look at it, so I'll go through his comment(s?) | 08:15 |
| tristan | does it do the `pytest.mark` approach and put everything into tests/ ? | 08:15 |
| tristan | curiously ? | 08:15 |
| tlater | Yes, it does | 08:15 |
| tristan | nice | 08:15 |
| * tlater finds that approach ends up very neat | 08:15 | |
| tristan | yeah, we can make the .gitlab-ci.yml cleaner too this way; simplifying the coverage report collection | 08:16 |
| tristan | and just have one test run per platform | 08:16 |
| tristan | which, we will be adding more of | 08:16 |
| tlater | Yup. The only downside is that *all* tests now take a long time | 08:16 |
| tristan | Eh ? | 08:17 |
| tristan | Whadayamean ? | 08:17 |
| tlater | You don't 10 minute feedback on unit tests anymore | 08:17 |
| tlater | You have to wait until the integration tests are done too | 08:17 |
| tristan | Oh you just mean, if you're the type who likes to sit and watch gitlab do stuff ? | 08:17 |
| tristan | yeah | 08:17 |
| tristan | To be frank; I 100% expect that anyone who pushes a branch for an MR, has run the local tests (sans the long integration ones) locally before ever submitting a branch | 08:18 |
| tristan | So, I have zero sympathy | 08:18 |
| tlater | I suppose I notice it a bit more because I'm actually testing if the the tests run on gitlab ;) | 08:19 |
| tristan | yeah :) | 08:19 |
| tristan | certainly you would hehe | 08:19 |
| tristan | juergbi, I provided some guidance for jmac to fix our stomping on unhandled exceptions, would be nice to get another proof read there: https://gitlab.com/BuildStream/buildstream/merge_requests/240#note_54878081 | 08:24 |
| tristan | it's a bit shorter than the one yesterday dont worry ;-) | 08:24 |
| juergbi | ok ;) | 08:24 |
| tristan | in case you may have better ideas there | 08:24 |
| gitlab-br-bot | buildstream: merge request (juerg/source-state->master: source state updates) #230 changed state ("merged"): https://gitlab.com/BuildStream/buildstream/merge_requests/230 | 08:27 |
| *** valentind has quit IRC | 08:27 | |
| tlater | Hrm, I'm pretty sure we are running out of space now, after all | 08:28 |
| tlater | We go from 5% usage to 78% usage of the big storage when running the normal linux tests | 08:29 |
| tlater | I guess tarballs are less efficient storage than ostree | 08:29 |
| tristan | They most certainly are. | 08:29 |
| * tlater thinks this could be resolved if we removed the pytest datadirs after each tests | 08:30 | |
| tristan | tlater, a workaround would be to delete the artifacts we dont reuse throughout the test | 08:30 |
| tristan | tlater, removing the whole artifact cache every time, would slow things down immensely | 08:30 |
| tlater | tristan: No, I think the issue here is that we don't share artifacts - we create individual @pytest.mark.datafiles things for each test | 08:31 |
| tristan | because you never reuse the base runtime | 08:31 |
| tlater | We only share sources | 08:31 |
| tristan | Ohhhh | 08:31 |
| * tlater is unsure how he would go about sharing the base platform artifact | 08:31 | |
| tlater | But I think that's my best option at this point | 08:31 |
| tristan | tlater, for artifact tests could we work around this in a way that is not with the cache ? | 08:31 |
| tristan | tlater, we already cache the sources with a custom buildstream.conf | 08:31 |
| tlater | There's no way to import a checkout, is there? | 08:32 |
| tristan | we need only route the artifact cache (using the config) to the same volume, but outside of the cache | 08:32 |
| tlater | Ah, I see what you mean | 08:32 |
| tlater | Yeah, that could work | 08:32 |
| tlater | It would still be neat to cache the base platform artifact | 08:33 |
| tlater | Though I guess that isn't necessary | 08:33 |
| tristan | tlater, We want to make sure that we only use this custom source & artifact cache location when running integration tests | 08:33 |
| tristan | dont let the existing tests inherit any of this | 08:33 |
| * tlater uses a separate 'CLI' decorator for integration tests | 08:33 | |
| tristan | That works :) | 08:33 |
| gitlab-br-bot | buildstream: merge request (175-refactor-integration-tests->master: WIP: Resolve "Refactor integration tests") #233 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/233 | 08:39 |
| *** cs_shadow has quit IRC | 08:45 | |
| *** m_22[m] has quit IRC | 08:45 | |
| *** ernestask[m] has quit IRC | 08:45 | |
| *** cgmcintyre[m] has quit IRC | 08:45 | |
| *** kailueke[m] has quit IRC | 08:45 | |
| *** jjardon[m] has quit IRC | 08:45 | |
| *** mrmcq2u[m] has quit IRC | 08:45 | |
| *** pro[m] has quit IRC | 08:45 | |
| *** ptomato[m] has quit IRC | 08:45 | |
| *** waltervargas[m] has quit IRC | 08:45 | |
| *** bochecha has quit IRC | 08:45 | |
| *** mattiasb has quit IRC | 08:45 | |
| *** cs_shadow has joined #buildstream | 08:45 | |
| *** m_22[m] has joined #buildstream | 08:45 | |
| *** ernestask[m] has joined #buildstream | 08:45 | |
| *** cgmcintyre[m] has joined #buildstream | 08:45 | |
| *** kailueke[m] has joined #buildstream | 08:45 | |
| *** jjardon[m] has joined #buildstream | 08:45 | |
| *** mrmcq2u[m] has joined #buildstream | 08:45 | |
| *** pro[m] has joined #buildstream | 08:45 | |
| *** ptomato[m] has joined #buildstream | 08:45 | |
| *** waltervargas[m] has joined #buildstream | 08:45 | |
| *** bochecha has joined #buildstream | 08:45 | |
| *** mattiasb has joined #buildstream | 08:45 | |
| *** csoriano has quit IRC | 08:45 | |
| *** csoriano has joined #buildstream | 08:46 | |
| juergbi | tristan: exception handling proposal seems reasonable to me. what we can would be handled properly and for the unexpected we would still get a sensible log | 08:53 |
| tristan | juergbi, yeah the only thing I can see is that if we start getting unhandled exceptions from the logging, or from terminating jobs, we may trigger infinite loops | 08:54 |
| tristan | but we could uninstall the global exception handler after it triggers once | 08:55 |
| tristan | in any case, thinking that far ahead is a bit too far :) | 08:55 |
| juergbi | or try except inside the global handler | 08:55 |
| tristan | well, that will run out of context when calling scheduler.terminate_jobs() and return + expect clean exit | 08:56 |
| tristan | in any case, we still reduce the possibilities to a very small section of code | 08:57 |
| juergbi | ah, true, it can't be synchronous with the loop | 08:57 |
| tristan | Catching unexpected output from children is equally important, and more tricky to implement | 08:58 |
| tlater | Unix tests passing \o/ - they also only take 20 minutes now | 09:01 |
| tlater | A minute faster than the linux tests, somehow - test coverage is similar to before | 09:03 |
| tlater | Oh, hrm, test coverage went down 10% | 09:04 |
| tristan | tlater, while you are swimming in test refactoring, please remember to file bugs for the things you havent covered | 09:07 |
| tristan | tlater, one thing comes to mind, now we're testing unix backend in a location sans ostree right ? | 09:08 |
| tlater | Yep | 09:08 |
| tristan | So, we would like to be sure that the installation with pip / setup.py works as expected and be able to remove our secret force backend env var | 09:09 |
| tristan | which is something a user should never have to know about when actually installing on a unix variant without bubblewrap & ostree | 09:10 |
| tlater | Yep, that's worth a bug - probably a hard test case to write, unfortunately | 09:11 |
| tristan | tlater, the comment was however mostly intended to separate tasks; we dont want to block your branch needlessly, but we must have a record of things which need doing | 09:11 |
| tristan | tlater, not a hard test case | 09:12 |
| tristan | tlater, actually I dont care for it being a "test case" either | 09:12 |
| tristan | tlater, currently the .gitlab-ci.yml ensures that we aways run tests from a dist tarball, which we install as part of the job | 09:12 |
| tristan | so, that part just has to work | 09:13 |
| tristan | (but, it doesnt have to block your branch, of course) | 09:13 |
| *** ssam2 has joined #buildstream | 09:14 | |
| tristan | tlater, its enough to abolish the force backend env var entirely; and have the unmodified tests continue to work | 09:14 |
| tlater | Hm, I don't think this would actually test that without running on an actual non-linux platform, though I don't remember how we detected the system we're running on anymore. | 09:15 |
| tristan | tlater, running a non-linux platform in CI is also a must | 09:15 |
| tlater | Yeah, that's probably where this is going | 09:15 |
| tristan | tlater, maybe should be punted to the BSD stuff | 09:15 |
| * tlater will make notes about this in issues | 09:16 | |
| tristan | thanks for that :) | 09:16 |
| tlater | Huh. I'm getting a lot of 'No source code for <source path>' errors now - presumably why test coverage went down | 09:21 |
| tlater | The paths refer to the build directory, while the paths with working coverage are in /usr/lib | 09:22 |
| tlater | I assume this might be due to the sdist installation? | 09:22 |
| *** adds68__ has quit IRC | 09:25 | |
| gitlab-br-bot | buildstream: merge request (175-refactor-integration-tests->master: WIP: Resolve "Refactor integration tests") #233 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/233 | 09:27 |
| gitlab-br-bot | buildstream: issue #196 ("source_dist: job fail with obscure message when a dependency is not satisfied") changed state ("closed") https://gitlab.com/BuildStream/buildstream/issues/196 | 09:36 |
| gitlab-br-bot | buildstream: merge request (docs-reorganization->master: Documentation: Restructured toplevel documentation) #241 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/241 | 10:12 |
| *** jonathanmaw has joined #buildstream | 10:13 | |
| tristan | tlater, sdist installation has been happening for a while | 10:15 |
| tristan | without damaging coverage reports it would seem | 10:15 |
| *** valentind has joined #buildstream | 10:22 | |
| *** aiden has joined #buildstream | 10:35 | |
| aiden | o/ | 10:35 |
| ssam2 | hi aiden | 10:35 |
| aiden | hi ssam2 | 10:36 |
| ssam2 | tlater, with respect to capturing and echoing the stdout from the tests ... | 10:36 |
| ssam2 | it's possible if we can inject our own code | 10:36 |
| tlater | ssam2: Yeah, it is *possible*, but may be very very messy | 10:36 |
| tlater | I'll look at it | 10:37 |
| tlater | But not sure I'll *like* it | 10:37 |
| ssam2 | it's seldom nice to do, indeed | 10:37 |
| ssam2 | it becomes nicer in python 3.5 as you can use async functions to do it | 10:38 |
| ssam2 | e.g. https://gitlab.com/baserock/definitions/blob/master/scripts/test-minimal-system#L40 (that's doing something else, but the principle is the same) | 10:38 |
| ssam2 | pretty sure we can't do that in python 3.4 though | 10:38 |
| tlater | Aww, I really wanted to try out async functions :| | 10:39 |
| *** aiden has quit IRC | 10:53 | |
| *** aiden has joined #buildstream | 10:55 | |
| *** valentind has quit IRC | 10:56 | |
| *** aidenhjj has joined #buildstream | 10:59 | |
| *** aiden has quit IRC | 11:00 | |
| *** aiden has joined #buildstream | 11:03 | |
| gitlab-br-bot | buildstream: merge request (175-refactor-integration-tests->master: WIP: Resolve "Refactor integration tests") #233 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/233 | 11:05 |
| gitlab-br-bot | buildstream: merge request (docs-reorganization->master: Documentation: Restructured toplevel documentation) #241 changed state ("merged"): https://gitlab.com/BuildStream/buildstream/merge_requests/241 | 11:07 |
| aiden | tlater, just fyi https://stackoverflow.com/questions/37465816/async-with-in-python-3-4 | 11:13 |
| tlater | ta aiden, maybe I can abuse this to learn about async after all :) | 11:16 |
| *** adds68 has joined #buildstream | 11:28 | |
| tristan | tlater, should we be blocking these enhancements just because you can't watch the buildstream log progress during integration style tests ? | 11:29 |
| tlater | No, mostly blocking because coverage isn't working for unix tests | 11:29 |
| tristan | ah | 11:30 |
| tlater | I suspect it's because we change users | 11:30 |
| tlater | But not sure yet - hard to debug due to gitlab-only issue :| | 11:30 |
| tristan | that should be easy enough; surely it must just be the coverage reports that are not being combined properly | 11:30 |
| tlater | No, the unix tests relate to the wrong path | 11:31 |
| tristan | Is it a wrong path ? | 11:31 |
| tlater | For some reason the coverage is about a locally installed buildstream, rather than the system installed buildstream that is present in the final coverage step | 11:31 |
| tlater | So when we finally combine half the coverage is thrown away because pytest can't find the code | 11:32 |
| tlater | It's an odd one | 11:32 |
| tristan | tlater, you are at least familiar with https://gitlab.com/BuildStream/buildstream/blob/master/.coveragerc#L15 right ? | 11:32 |
| tlater | Yep | 11:32 |
| tristan | okay | 11:32 |
| tlater | The issue is this: https://gitlab.com/BuildStream/buildstream/-/jobs/48311031 | 11:33 |
| tlater | ^ Just coverage for the unix tests | 11:33 |
| tlater | When running both, we get a report, but pytest throws any unix coverage away: https://gitlab.com/BuildStream/buildstream/-/jobs/48282600 | 11:33 |
| jonathanmaw | I'm looking at adding a test for checking that bzr workspaces are being initialised properly. The only way to be sure involves invoking 'bzr'. Should I be using subprocess.check_output() or is there a better way to shell out during tests? | 11:34 |
| jonathanmaw | I had a look at the "cli" helper, but that seems to be only usable to invoke "bst" | 11:34 |
| * tlater is pretty sure subprocess is the only option | 11:35 | |
| tristan | tlater, use this: https://gitlab.com/BuildStream/buildstream/blob/master/.gitlab-ci.yml#L179 here: https://gitlab.com/BuildStream/buildstream/blob/master/.gitlab-ci.yml#L200 | 11:35 |
| tristan | tlater, that may well fix it, also; make sure you list source_dist in dependencies for the coverage task | 11:36 |
| tristan | tlater, I'm guessing something in your setup is causing the current approach to break | 11:37 |
| tlater | Hm, odd | 11:37 |
| tristan | we never used the dist tarball in the coverage combine step; but we generate the reports from the dist tarball | 11:37 |
| gitlab-br-bot | buildstream: merge request (175-refactor-integration-tests->master: WIP: Resolve "Refactor integration tests") #233 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/233 | 11:37 |
| *** aiden has quit IRC | 11:38 | |
| tristan | tlater, also; you could theoretically reproduce that step locally to verify; I did that originally to create the coverage aggregation step | 11:38 |
| tlater | I hope we don't accidentally run tests against the docker-installed buildstream | 11:38 |
| tristan | by downloading the artifacts and running coverage combine and looking at the results | 11:39 |
| *** aiden has joined #buildstream | 11:39 | |
| tristan | tlater, my guess is that we never run tests using the installed stuff; which is another bug | 11:39 |
| tristan | tlater, assuming that you're using something similar to the runcli.py fixture to launch the integration style tests | 11:40 |
| tristan | the existing integration tests will be using the installed bst; but the `./setup.py test` tests only use local stuff and dont ever require installation | 11:41 |
| tlater | Yeah, that's a point | 11:41 |
| tristan | tlater, an acceptable regression considering all your improvements, needs a report, though. | 11:42 |
| *** aidenhjj has joined #buildstream | 11:42 | |
| tlater | In general, I think figuring out what pytest resolves 'buildstream' to might be something to investigate | 11:42 |
| tristan | I suppose ideally we make the integration decorated tests require installation | 11:42 |
| tristan | and use the installed one for those tests | 11:42 |
| tristan | ensuring we have good coverage of both in-tree and installed | 11:43 |
| tristan | tlater, if pytest is using anything except what is in the source tree; that would be an incredibly horrible bug | 11:43 |
| tristan | I really, really doubt that. | 11:43 |
| tlater | It's possible, it relies on python imports | 11:44 |
| tristan | More likely that we can possibly make that mistake, but pytest should to some degree ensure that never happens | 11:44 |
| tlater | Which are a mess if you have multiple locations of your package | 11:44 |
| *** aiden has quit IRC | 11:44 | |
| * tlater tests it anyway just to be sure | 11:45 | |
| tristan | tlater, running tests on a site where an installed copy of your module exists has to be really, really common | 11:45 |
| tristan | I dont think anyone would recommend py.test if that was a known bug | 11:45 |
| tristan | (and py.test is the defacto standard) | 11:45 |
| tlater | Hm, I suppose | 11:46 |
| tlater | Still, I'm not sure how much they mess with default behaviour | 11:46 |
| tristan | isolated black boxes man... question it if there is a bug, but assuming it works properly is a good start. | 11:47 |
| * tlater questions it because he saw the same test suite running two different versions of the same package, but will concede | 11:48 | |
| tristan | for our CI then; we should ensure that the docker image never has buildstream installed as a start. | 11:49 |
| tristan | just remove any doubt at the source | 11:49 |
| tlater | Yep, that seems reasonable | 11:49 |
| tlater | ssam2: Is your comment on contains() done? | 11:50 |
| * tlater thinks changing it to be more intuitive probably isn't a poor idea | 11:51 | |
| jonathanmaw | tristan: my current implementation of a test that initialising workspaces with bzr is behaving properly has highjacked the test_open test with a special case for if the kind is "bzr". Should I separate it out? | 11:52 |
| jonathanmaw | Yeah, I'm going to do that, redundancy in tests is a minor problem, but making the tests an unreadable mess is a major one, so I should try to prevent it. | 11:54 |
| *** aidenhjj has quit IRC | 12:03 | |
| *** aiden has joined #buildstream | 12:07 | |
| aiden | tox is good for isolated testing | 12:08 |
| tristan | jonathanmaw, "has highjacked test_open" ... I have no idea what you're talking about | 12:09 |
| *** aidenhjj has joined #buildstream | 12:10 | |
| persia | tox is also good for isolated execution | 12:10 |
| ssam2 | tlater, yes | 12:10 |
| tristan | We had a virtualenv segment of the instructions, and threw that in the trash where it belongs | 12:12 |
| *** aiden has quit IRC | 12:12 | |
| tlater | Heh, poor virtualenv | 12:12 |
| tristan | venv doesnt work well for us, because we need to use the flag which allows it to also see the system python | 12:12 |
| tristan | kind of obsoleting the advantage of using venv | 12:12 |
| ssam2 | yes, i wonder if tox would have the same issue | 12:12 |
| ssam2 | they are good if your deps are pure python, but not if you depend on system-installed python packages which can't be obtained from Pip | 12:13 |
| tristan | which seems to work for those who live in that `everything is pure python` bubble | 12:13 |
| tristan | so yeah, I expect tox to be in the same pile of trash | 12:13 |
| persia | tox supports non-pure python, but yeah, it's likely not entirely appropriate for BuildStream, as there are many needs for deep system things. | 12:14 |
| jonathanmaw | tristan: I'm picking up the bzr-specific changes for https://gitlab.com/BuildStream/buildstream/issues/53, and making sure I've got tests in place for it | 12:15 |
| jonathanmaw | my first implementation of testing it was to add a special case in test_open, so I could reuse their call to open_workspace | 12:15 |
| jonathanmaw | but I'm having second thoughts about it, and I'd rather discuss it now than wait for it to be brought up in review of a merge request | 12:16 |
| tristan | I dont even know what test_open is | 12:16 |
| tristan | jonathanmaw, that said; I'd much prefer more verbose tests, than lines of common code that say `if this_thing is "bzr"` | 12:17 |
| tristan | if that's your question | 12:17 |
| jonathanmaw | okie doke | 12:17 |
| gitlab-br-bot | buildstream: merge request (175-refactor-integration-tests->master: WIP: Resolve "Refactor integration tests") #233 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/233 | 12:21 |
| tristan | ssam2, check out http://buildstream.gitlab.io/buildstream/ | 12:22 |
| ssam2 | nice, will take a bit of getting used to for people who have memory mapped the existing layout | 12:23 |
| ssam2 | but no doubt less daunting for first time readers | 12:23 |
| tristan | I like how all the plugin format docs now show up on the same main page as the core format docs | 12:24 |
| tristan | And of course, now there is a sensible place to put more user specific guide-style docs | 12:25 |
| *** adds68 has quit IRC | 12:26 | |
| * tlater finally knows where to place his doc \o/ | 12:26 | |
| ltu | I guess !206 still needs to be re-based before merging - https://gitlab.com/BuildStream/buildstream/merge_requests/206 | 12:26 |
| tristan | you really love that bug I know | 12:27 |
| tristan | it's become an obsession :) | 12:27 |
| ltu | yeah...i won't deny it! | 12:28 |
| tristan | ltu, frankly I was planning to take an afternoon and just check all the related branches locally and see what parts of it are usable and just massage it into the docs | 12:29 |
| ltu | tristan, ok, great | 12:31 |
| ltu | my thoguhts here are about FOSDEM | 12:32 |
| ltu | Better to go there with really good introductory documentation for newcomers | 12:32 |
| *** mcatanzaro has joined #buildstream | 12:48 | |
| gitlab-br-bot | buildstream: merge request (175-refactor-integration-tests->master: WIP: Resolve "Refactor integration tests") #233 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/233 | 12:50 |
| gitlab-br-bot | buildstream: merge request (175-refactor-integration-tests->master: WIP: Resolve "Refactor integration tests") #233 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/233 | 12:54 |
| gitlab-br-bot | buildstream: merge request (164-minimise-overlaps-by-having-overlaps-raise-exceptions-unless-configured-not-to->master: Resolve "Minimise overlaps by having overlaps raise exceptions unless configured not to") #181 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/181 | 12:59 |
| tristan | jonathanmaw, I was trying to finally merge the above MR 181 | 13:08 |
| tristan | jonathanmaw, and found that one thing with a silly illegal `element-path` trying to escape it's project directory | 13:09 |
| jonathanmaw | tristan: okie doke | 13:09 |
| tristan | jonathanmaw, but I may make a couple of more cosmetic comments now that I know I cant merge | 13:09 |
| tristan | stylistic I should say | 13:09 |
| tristan | ok, yeah that ElementError correction is more than cosmetic too, better fix that | 13:11 |
| tristan | hmmm, looks like this also adds work to the junctions, unless I make jonathanmaw do it properly first | 13:17 |
| tristan | where properly means "with knowledge that junctions are coming" | 13:17 |
| jonathanmaw | tristan: ok, I'll have a look at doing that. Is there a MR for junctions that I can look at to see how to do it properly? | 13:23 |
| tristan | jonathanmaw, I'm explaining it in the comment as we speak... | 13:24 |
| tristan | gimme 3 minutes... | 13:24 |
| jonathanmaw | :) ta | 13:24 |
| gitlab-br-bot | buildstream: merge request (175-refactor-integration-tests->master: WIP: Resolve "Refactor integration tests") #233 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/233 | 13:26 |
| * tlater wonders if there is some sort of a .gitlab-ci.yaml runner that would help testing this - stupid mistakes should not add 20 minutes of idle time :| | 13:27 | |
| tristan | jonathanmaw, https://gitlab.com/BuildStream/buildstream/merge_requests/181/diffs#note_54934756 | 13:28 |
| tristan | juergbi, ssam2; since you are both deep massaging junctions into place it seems, you may want to take a look at the above comment too ^^^ | 13:28 |
| tristan | Probably we will want to ensure we have a test case post-landing-of-junctions to ensure the overlap-on-error behavior with dependant projects in play behaves as expected | 13:29 |
| jonathanmaw | tristan: okie doke | 13:29 |
| tristan | Alright all comments up for that | 13:40 |
| gitlab-br-bot | buildstream: merge request (175-refactor-integration-tests->master: WIP: Resolve "Refactor integration tests") #233 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/233 | 13:54 |
| tlater | Eugh, I think it will be neater just to write my own stdout/err redirection than to subclass several pytest internal classes | 14:04 |
| * persia wonders if it might be worth considering an alternate test harness, if pytest fits so badly | 14:07 | |
| tlater | I looked at some stackoverflow questions asking the same, apparently all others have the same issue | 14:08 |
| tlater | 'teeing' is just not something python test library developers think about. | 14:08 |
| tlater | For everything else, pytest seems fine, so it would be a bit rough to throw it out over this | 14:09 |
| persia | Reading the overlap review, I wonder if behaviour should differ in cases where overlapped files are identical vs. different | 14:11 |
| *** tristan has quit IRC | 14:13 | |
| tlater | Coverage is still not correct, it really looks like the unix integration tests just run the wrong buildstream installation | 14:33 |
| *** tristan has joined #buildstream | 14:37 | |
| tristan | persia, I dont think behavior should differ in case of exact matchliness no | 14:46 |
| persia | tristan: Because the user should understand what is happening if they say "no overlaps", and fix it? | 14:47 |
| tristan | persia, A.) It's still a case that I want to be told about, if either a warning or error, B.) It can be whitelisted like anything else, and C.) It would be quite a performance hit as well | 14:48 |
| *** ChanServ sets mode: +o tristan | 14:48 | |
| persia | Ah, yes, the performance aspect is one that makes more of a difference. I no longer wonder :) | 14:49 |
| tristan | heh | 14:49 |
| tlater | I really don't understand why this happens: https://gitlab.com/BuildStream/buildstream/-/jobs/48343936 | 14:50 |
| tlater | The first set of tests seems to run bst from `/builds/BuildStream/buildstream`, while the second runs them from `/usr/lib/python3.6/site-packages/buildstream` | 14:50 |
| tlater | As a result, coverage.py gets confused and misreports coverage | 14:51 |
| tristan | tlater, then click on the separate jobs, download the artifacts, and run coverage combine yourself | 14:51 |
| tristan | tlater, to be honest I dont understand why the dist would be needed, I suggested you try that because of paths showing up with dist in them | 14:55 |
| tristan | but really, coverage combine, afaics, should not really need the source code | 14:56 |
| tristan | at all | 14:56 |
| *** mcatanzaro has quit IRC | 14:56 | |
| *** mcatanzaro has joined #buildstream | 14:56 | |
| gitlab-br-bot | buildstream: merge request (175-refactor-integration-tests->master: WIP: Resolve "Refactor integration tests") #233 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/233 | 14:57 |
| tristan | tlater, it may also be that you added the cd dist/ but failed to adjust the ../../ leading to the .coveragerc below; which is exposing an entirely different bug | 14:57 |
| tristan | tlater, i.e. the bug that .coveragerc is not listed in MANIFEST.in afaics | 14:58 |
| tristan | i.e. the dist tarball doesnt have the special sauce telling coverage how to match the paths in these reports together | 14:58 |
| tlater | No, the .coverage files are copied to the cwd | 14:58 |
| tristan | You are not reading what I am typing. | 14:58 |
| tlater | I also did adjust the --rcfile paths | 14:59 |
| tlater | So that shouldn't cause this issue - you are right that the .coveragerc isn't in the dist directory though | 14:59 |
| tristan | That is what I am typing yes, so basically, you are using the .coveragerc from the parent directory (gitlab's checkout) | 14:59 |
| tristan | Instead of the .coveragerc which should be in the dist tarball but is currently missing. | 15:00 |
| tlater | Would that .coveragerc be different? | 15:00 |
| * tristan should fix that straight away actually | 15:00 | |
| *** mcatanzaro has quit IRC | 15:00 | |
| tristan | tlater, the missing .coveragerc would be entirely different | 15:00 |
| tristan | like 0 bytes and no directory entry different. | 15:00 |
| tlater | Right, then that is an issue. | 15:01 |
| *** mcatanzaro has joined #buildstream | 15:01 | |
| tristan | nope | 15:01 |
| tristan | tlater, I cannot explain why, but it seems to be included | 15:02 |
| gitlab-br-bot | buildstream: merge request (175-refactor-integration-tests->master: WIP: Resolve "Refactor integration tests") #233 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/233 | 15:02 |
| * tlater got errors trying to include it from cwd | 15:02 | |
| tristan | It *is* missing from the MANIFEST.in | 15:02 |
| *** mcatanzaro has quit IRC | 15:02 | |
| tlater | Or maybe I just adjusted it for no reason... | 15:02 |
| tristan | when I run ./setup.py sdist here, it ends up in the tarball | 15:02 |
| *** mcatanzaro has joined #buildstream | 15:02 | |
| tristan | Maybe something spooky in the setup.py "automates" it's inclusion; so we can all have the luxury of not knowing what's going on | 15:03 |
| gitlab-br-bot | buildstream: merge request (175-refactor-integration-tests->master: WIP: Resolve "Refactor integration tests") #233 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/233 | 15:03 |
| gitlab-br-bot | buildstream: merge request (175-refactor-integration-tests->master: WIP: Resolve "Refactor integration tests") #233 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/233 | 15:03 |
| tlater | That's nice of setuptools | 15:04 |
| * tlater still suspects pytest is running the wrong buildstream, though | 15:04 | |
| tristan | I would think perhaps something about test_requiring coverage.py causes coverage's internal mumbo jumbo to include it | 15:05 |
| tristan | /usr/lib/python3.6/site-packages/buildstream/_artifactcache/tarcache.py 131 21 83.97% | 15:06 |
| tristan | tlater, it does indeed seem to choose the installed one | 15:06 |
| tristan | tlater, however, we did install it | 15:06 |
| tristan | I dont disagree that it's ambiguous, and in a pytest context should never happen | 15:07 |
| tlater | Even if we do install it, coverage probably gets confused because the file paths differ | 15:07 |
| tlater | And therefore misreports coverage in the end | 15:08 |
| tristan | file paths differing over several runs is *entirely* expected | 15:08 |
| tristan | but you already mentioned that you are aware of that | 15:08 |
| tristan | <tristan> tlater, you are at least familiar with https://gitlab.com/BuildStream/buildstream/blob/master/.coveragerc#L15 right ? | 15:09 |
| tristan | <tlater> Yep | 15:09 |
| tristan | <tristan> okay | 15:09 |
| tristan | Just incase ^^^ | 15:09 |
| tlater | Right, I forgot again | 15:09 |
| tlater | I guess all I can do is wait for this CI to finish and see if using the coveragerc from the sdist works | 15:10 |
| tristan | tlater, no, you can download the artifacts from the job pages on gitlab | 15:11 |
| tristan | tlater, and you can modify your local .coveragerc, and tweak it so that it works. | 15:11 |
| tristan | by running it manually | 15:11 |
| tristan | without waiting for gitlab to slowly do it | 15:12 |
| tlater | Oh, right, I can adjust the path to also accept /src | 15:12 |
| tlater | o\ | 15:12 |
| gitlab-br-bot | buildstream: merge request (175-refactor-integration-tests->master: WIP: Resolve "Refactor integration tests") #233 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/233 | 15:13 |
| tristan | e.g. download button on the right: https://gitlab.com/BuildStream/buildstream/-/jobs/48343934 | 15:13 |
| gitlab-br-bot | buildstream: merge request (175-refactor-integration-tests->master: WIP: Resolve "Refactor integration tests") #233 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/233 | 15:14 |
| tlater | Yeah, I just got coverage to complain about paths again | 15:14 |
| tlater | But forgot to set the path to the one I have in my image | 15:14 |
| *** mcatanzaro has quit IRC | 15:20 | |
| *** mcatanzaro has joined #buildstream | 15:20 | |
| *** aidenhjj has quit IRC | 15:32 | |
| tlater | Ah | 15:33 |
| tlater | Apparently you need to run 'combine' on both files | 15:33 |
| tlater | Otherwise it doesn't translate the paths | 15:33 |
| tlater | Weird that 'report' wouldn't have that functionality | 15:34 |
| tristan | the path matching happens at combine time | 15:35 |
| tristan | it's what informs combine how to function | 15:35 |
| gitlab-br-bot | buildstream: merge request (175-refactor-integration-tests->master: WIP: Resolve "Refactor integration tests") #233 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/233 | 15:35 |
| tlater | Yes, but I *assume* that the `-a` flag is something along the lines of 'append', which means that coverage doesn't translate what is already there | 15:36 |
| tlater | It just translates all appended paths to something it considers the default path | 15:36 |
| tlater | At least, that's my best guess from playing with it | 15:36 |
| tlater | We just happened to always have the default path in the first file we copied | 15:37 |
| tlater | Either way, I think I have a script that should do it properly now, regardless of what actually was going wrong | 15:37 |
| * tlater is pretty sure his interpretation is correct after looking at .coverage in various stages of translation | 15:42 | |
| tristan | tlater, yeah I can imagine that could very well be | 15:42 |
| *** aiden has joined #buildstream | 15:51 | |
| *** aidenhjj has joined #buildstream | 15:51 | |
| *** aiden has quit IRC | 15:54 | |
| *** aidenhjj is now known as aiden | 15:56 | |
| tlater | Finally, test coverage is exactly the same it was before \o/ | 15:59 |
| tlater | Heh, no, actually .09% higher | 16:00 |
| tristan | tlater, it's also somehow racy :) | 16:00 |
| tristan | kind of scary and freaky | 16:00 |
| tristan | tlater, try manually running a few pipelines on the same commit; get different results | 16:01 |
| * tlater really hopes this might have changed with the new test runs - though it might have been due to the artifact caching, which we reverted to doing again | 16:04 | |
| tristan | would be nice | 16:05 |
| tristan | but it's possible that it's racy, and that some few lines of code (probably the very same ones), have time to run or not in a child task | 16:06 |
| tristan | hopefully its not that likely, but I'd have to investigate exactly which lines they were | 16:06 |
| tristan | tlater, so it seems that you are probably right about pytest being pretty broken | 16:24 |
| tristan | tlater, and we may have to resort to integrating something like tox (uuughhhh, yet another fraking moving part !!!) | 16:24 |
| tristan | at least for the sake of developers running tests on their hosts | 16:25 |
| persia | Just don't have tox venv, and make sure you point it explicitly at the non-python bits, and it shouldn't be too painfukl. | 16:25 |
| tristan | persia, I dont compute | 16:26 |
| persia | I was making suggestions based on my experience fighting with tox, that may or may not apply. Feel free to ignore. | 16:26 |
| tristan | persia, for CI, it's trivial enough to ensure the docker image does not ever have buildstream installed | 16:27 |
| tristan | persia, on a developer's host, there is no such guarantee | 16:27 |
| tristan | persia, without "extra explicit isolation using venv and/or tox"... python developers seem to think that there is no expectation that pytest; when integrated as recommended by pytest folks, would choose to run the buildstream copy in your checkout | 16:28 |
| tristan | rather than choosing another one randomly sitting in /usr/lib/python${version}/ | 16:28 |
| persia | Ah, yes, one would need not to have any version of buildstream actually installed for the tests to work without venv. | 16:31 |
| gitlab-br-bot | buildstream: merge request (175-refactor-integration-tests->master: WIP: Resolve "Refactor integration tests") #233 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/233 | 16:32 |
| persia | But I thought that using venv made it hard to use some system tooling, and suspected that this might include some of our sandboxing strategies. | 16:32 |
| persia | I'd be delighted to be mistaken. | 16:32 |
| tlater | tristan: We can resort to following aiden's advice and write a custom import wrapper | 16:33 |
| tlater | But that's probably not that much better | 16:34 |
| tristan | persia, it needs to use the flag that allows including the system installed python libs as well | 16:40 |
| tlater | Would that not defy the purpose since importing buildstream could still be ambiguous? | 16:41 |
| tristan | persia, which means the isolation aint perfect like it would be with a pure python program, but probably fine for tests, as it should at least isolate our pure python requirements, including ourself | 16:41 |
| tristan | tlater, it should not be ambiguous for that, no; at least not as currently advertised | 16:42 |
| tristan | However, since the venv setup installation instructions were a load of bloat; and the isolation was imperfect anyway, it was alright to remove that installation approach from docs I think. | 16:42 |
| gitlab-br-bot | buildstream: merge request (175-refactor-integration-tests->master: WIP: Resolve "Refactor integration tests") #233 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/233 | 16:47 |
| tristan | Aha | 16:56 |
| tristan | jjardon[m], commit dd0d200d370211b18da7978c34962d9b71970b7a seems to have caused https://bugzilla.gnome.org/show_bug.cgi?id=792302 | 17:02 |
| tristan | btw | 17:02 |
| tristan | jjardon[m], cmake -B, is not shorthand for cmake --build | 17:02 |
| tristan | according to `cmake --help-full` at least | 17:03 |
| tristan | I'll investigate tomorrow | 17:03 |
| *** aiden has quit IRC | 17:10 | |
| jjardon[m] | tristan: yeah, Its an undocumented feature, juergbi and me discussed about using it in the MR | 17:11 |
| jjardon[m] | uh, the bst git history doesnt have links to the MR's, ie, merge commits? | 17:12 |
| tristan | jjardon[m], you must always specify issue number in commits man | 17:12 |
| jjardon[m] | several times there is not an issue associated to a MR | 17:13 |
| jjardon[m] | any reason to not use merge commits? I think it makes clearer what were the contents of a specific MR | 17:14 |
| jjardon[m] | found it! https://gitlab.com/BuildStream/buildstream/merge_requests/173#note_50145023 | 17:18 |
| jjardon[m] | tristan: let me know what you dinf and Im happy to rewrite the patch again | 17:18 |
| tristan | dinf ? | 17:19 |
| persia | jjardon[m]: If there isn't an issue associated with an MR, create an issue to justify the MR :) | 17:20 |
| jjardon[m] | persia: agreed, but thats not enforced so references can still be lost: a merge commit will always be there | 17:21 |
| persia | fair | 17:21 |
| tristan | jjardon[m], the referred bug report shows that EDS doesnt build this way, mcrha says he builds it with `cmake --build _builddir` | 17:21 |
| tristan | jjardon[m], the docs for `cmake -B` say that `cmake -B` is to specify a "package directory" | 17:22 |
| tristan | jjardon[m], so it is kind of a mystery to my how cmake even made it that far into the build without failing | 17:22 |
| tristan | but seems there is a subtle difference, or random similarity, with specifying the -B "package directory" and the --build "build directory" | 17:22 |
| tristan | I dont really understand from the MR comments why -B was chosen over --build | 17:23 |
| tristan | jjardon[m], on the other hand, mchra *also* uploaded an EDS patch to test, because the generated file is not entirely a normal thing | 17:23 |
| tristan | I'm not sure that the suggested patch to EDS should be applied, if we're not using exactly the right semantics to specify a build directory, though. | 17:24 |
| juergbi | tristan: -B as package directory is for cpack, not cmake, afaict | 17:24 |
| tristan | Unless, someone can explain with deeper knowledge "Why a cmake package directory can and should be used to specify the build directory implicitly" | 17:24 |
| jjardon[m] | yup, let me work in a patch; do you want me to open an issue? | 17:24 |
| tristan | juergbi, I only found documentation for -B in `cmake --help-full`, by searching for it | 17:25 |
| tristan | So you could be right yes | 17:25 |
| juergbi | yes, as jjardon[m] mentioned cmake -B is not documented | 17:25 |
| juergbi | but is available for 15 years | 17:25 |
| tristan | If -B in place of --build is *in fact* short hand for --build, then we need not really change it | 17:25 |
| juergbi | i thought --build was something else, need to recheck | 17:25 |
| tristan | I however tend to prefer the long options in build commands, rather than shorthand | 17:26 |
| tristan | maybe it's for something else ? | 17:26 |
| juergbi | cmake --build is to implicitly execute make or whatever underlying build tool is configured, afaict | 17:26 |
| juergbi | so called 'Build Tool Mode' | 17:26 |
| juergbi | not useful for us | 17:27 |
| juergbi | afaik, there is no long option available that is equivalent to -B | 17:27 |
| tristan | Oh I'm wrong about --build | 17:27 |
| tristan | I picked up the wrong comment from mchra | 17:27 |
| juergbi | i believe the only other option would be to use 'cd' before cmake | 17:27 |
| tristan | What he does is "mkdir _build && cd build && cmake <args>" | 17:28 |
| tristan | Ah | 17:28 |
| tristan | Ok, we should probably call it an EDS bug in this case, very confusing | 17:28 |
| * jjardon[m] still prepares an unrelated patch for the cmake build system | 17:30 | |
| jjardon[m] | to use ninja instead make :) | 17:30 |
| tristan | jjardon[m], Will that "just work" for every project that uses cmake ? | 17:32 |
| tristan | jjardon[m], or it should probably be optional right ? | 17:33 |
| jjardon[m] | well, gnome-continuous use ninja by default; agreed it should be optional | 17:33 |
| tristan | jjardon[m], I would think; default is make as per the cmake.yaml defaults, controlled by a variable in cmake.yaml | 17:35 |
| tristan | jjardon[m], that can be toggled project-wide anyway in project.conf | 17:35 |
| tristan | seems the safest | 17:35 |
| tristan | so easy to say "all of this project works with cmake + ninja" in one go | 17:36 |
| jjardon[m] | yeah | 17:36 |
| jjardon[m] | actually | 17:36 |
| jjardon[m] | mmm | 17:36 |
| tristan | at that point, if you've done that; and you have one element that doesnt work; the element.bst file itself anyway has the highest priority | 17:36 |
| tristan | so it can be backwards overridden again | 17:37 |
| jjardon[m] | we could use cmakeargs for the configure step but your still need to change the build and install commands | 17:37 |
| juergbi | maybe cmake --build would actually be useful here? | 17:39 |
| juergbi | i've never used it, though | 17:39 |
| tristan | probably cmake --build is a good idea for an initial try in api-unstable master dev branch | 17:40 |
| tristan | i.e. try the most elegant thing first; give it time to see if it actually sucks before committing to it in next stable | 17:41 |
| gitlab-br-bot | buildstream: merge request (jonathan/bzr-source-method->master: WIP: Jonathan/bzr source method) #242 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/242 | 17:42 |
| gitlab-br-bot | buildstream: merge request (version-read-no-pkg_res->master: WIP: Get version number w/o pkg_resources) #234 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/234 | 17:51 |
| gitlab-br-bot | buildstream: merge request (sam/integration-tests-alpine->master: WIP: Use a custom base sysroot for the integration tests) #243 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/243 | 18:07 |
| gitlab-br-bot | buildstream: merge request (sam/integration-tests-alpine->master: WIP: Use a custom base sysroot for the integration tests) #243 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/243 | 18:14 |
| *** jonathanmaw has quit IRC | 18:18 | |
| *** valentind has joined #buildstream | 18:53 | |
| gitlab-br-bot | buildstream: merge request (version-read-no-pkg_res->master: WIP: Get version number w/o pkg_resources) #234 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/234 | 18:54 |
| *** ssam2 has quit IRC | 18:57 | |
| *** ssam2 has joined #buildstream | 18:59 | |
| *** tristan has quit IRC | 21:11 | |
| *** ssam2 has quit IRC | 22:01 | |
| *** ssam2 has joined #buildstream | 22:14 | |
| *** valentind has quit IRC | 22:25 | |
| *** ssam2 has quit IRC | 23:12 | |
| *** persia has quit IRC | 23:18 | |
| *** persia has joined #buildstream | 23:18 | |
| *** persia has quit IRC | 23:53 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!