Friday, 2022-02-04

*** ysandeep|out is now known as ysandeep01:14
*** dviroel|afk is now known as dviroel01:16
*** dviroel is now known as dviroel|out01:22
*** ysandeep is now known as ysandeep|afk01:40
opendevreviewIan Wienand proposed zuul/zuul-jobs master: ensure-virtualenv: Don't support on CentOS 9-stream
opendevreviewIan Wienand proposed zuul/zuul-jobs master: ensure-virtualenv: Don't support on CentOS 9-stream
*** akahat is now known as akahat|rover05:29
*** ysandeep|afk is now known as ysandeep07:09
ianwfinally got the quotes right07:26
ianw(on 589)07:26
*** amoralej|off is now known as amoralej07:28
fricklerianw: are we sure that c9 and c9s will be identical in that regard? because you talk about c9s but only test for centos==907:38
ianwfrickler: aiui, there will not be a 9, and 9-stream reports itself as "9" (without the stream)08:12
ianwsimilar issue with $releasever set in
rpittaugood morning! I was talking the other day with fungi and clarkb about an issue with debian bullseye DIB images not getting ips from dhcp, I have to rectify myself, we don't use simple-init for the ipa ramdisk, we just use dhcp-all-interfaces08:14
rpittauI'm able to reproduce the issue locally, it looks like the interfaces with the predictable names are completely ignored08:14
rpittauusing net.ifnames=0 won't work as we pass kernel args by pxe, not by grub, also I guess this should just work without the need to do that08:14
*** akahat|rover is now known as akahat|lunch08:30
*** ysandeep is now known as ysandeep|lunch08:36
*** akahat|lunch is now known as akahat|rover09:22
mnasiadkaGood morning - is there a way to map a tarball from (e.g. $project-master.tar.gz) to a git commit hash? I can't seem to find any metadata in the tarball itself, that would help in that matter.10:00
*** ysandeep|lunch is now known as ysandeep10:04
*** pojadhav is now known as pojadhav|brb10:24
*** dviroel|out is now known as dviroel|ruck10:46
*** rlandy|out is now known as rlandy|ruck11:10
*** pojadhav|brb is now known as pojadhav11:14
*** ysandeep is now known as ysandeep|mtg11:17
opendevreviewRiccardo Pittau proposed openstack/diskimage-builder master: Move dhcp script to correct custom dir
*** ysandeep|mtg is now known as ysandeep12:02
fungimnasiadka: if the project uses pbr, then the hash is in the package metadata. you can use the `pbr freeze` subcommand to see it, or access the pbr.json metadata file directly13:29
fricklerfungi: we're not talking about wheels, but source tarballs13:48
frickleralthough there's a PKG-INFO in there, with e.g. "Version: 13.1.0.dev51", maybe that's already enough13:51
fricklermnasiadka: ^^13:51
mnasiadka51 commits from 13.1.0? well, it's a pointer, but as a Kolla user probably I wouldn't be the most happy guy on earth :)13:52
*** pojadhav- is now known as pojadhav13:53
fricklermnasiadka: well, it should be unique unless you have local patches, and in that case it is more helpful than 8966e65d probably. if you don't know in which repo+branch to look for it, the latter won't likely help you much, either13:58
*** ysandeep is now known as ysandeep|mtg13:59
mnasiadkafrickler: just trying to think how best to "find which point of Neutron stable/wallaby code I have running"13:59
fungifrickler: pbr puts its medatadata in sdist tarballs too14:13
fricklerfungi: seemingly only the version I mentioned above, not the git sha14:19
fricklerand if I do "pbr info designate" on an install from the tarball, it is also missing that part as compared to when installing from git directly14:20
fungihuh, i wonder if we stopped stashing the metadata in there and didn't notice14:21
fricklerfungi: that seems to be the case, stable-ocata contains designate.egg-info/pbr.json, master tarball doesn't14:25
fungiyeah, i have a feeling something changed in setuptools/pip causing us to silently stop including it. i'll dig deeper14:25
fungia bit of spelunking in one of my personal projects which always uses the latest of everything was including a pbr.json in sdist tarballs as recently as august of last year14:42
fungigoing to see if i can narrow down the timeframe any better than that14:42
fungibut that indicates it's been a fairly recent regression, at least14:42
fungimakes me wish i'd been releasing that project early and often so i'd have a tighter window on it14:45
*** ysandeep|mtg is now known as ysandeep|out14:53
fricklerfungi: checking designate tarballs, from 2021-10-06 is good, stable tarballs from 2021-12-02 are not, so that's 2 months timeframe14:56
fungithanks, that gives me a much smaller number of pbr, setuptools and pip versions to check14:58
fungiassuming we used latest of those at the time. i might need to factor tox/virtualenv versions into that since they can influence what version of setuptools is used14:58
opendevreviewMerged openstack/project-config master: Add cirros/cirros project
fricklerkolla has some rc releases in that timeframe, I'm going to check those, too14:59
fricklero.k., so that looks like the change happened between 2021-11-05 and 2021-11-19, kolla vs but of course it may have been a change in our tooling rather than upstream versions15:02
fungiright, but that's only two weeks to check at least. i'll be able to dig deeper after the d&i wg meeting is over15:07
frickler looks as expected, is there anything specific we wanted to check in term of the main branch? I'm going to add myself to cirros-core now and see if there's a small job I can set up other than noop15:11
fungiyeah, zuul job configuration was the thing we're needing to test15:13
*** dviroel|ruck is now known as dviroel|ruck|lunch15:17
Clark[m]fungi: if I had to guess setuptools stopped calling the hook that generates that file on sdist but does call it for wheels15:41
fungiClark[m]: it's not happening for wheels either though15:41
fungifrom what i can see15:41
Clark[m]Oh I missed that. Maybe they dropped the hook entirely then?15:42
Clark[m]I guess bisect would hopefully figure it out if it was a pbr change pretty easily but harder if new setuptools behavior15:43
*** akahat|rover is now known as akahat|dinner15:45
fricklerso this looks good to me, gonna step away for a bit and then try to do a real job
fungiokay, my meetings are done for the day. i've got a couple other quick things i need to knock out, and then i can start looking more closely at the pbr metadata problem16:18
*** dviroel|ruck|lunch is now known as dviroel|ruck16:22
*** rlandy|ruck is now known as rlandy|ruck|brb16:30
*** marios is now known as marios|out16:38
*** rlandy|ruck|brb is now known as rlandy|ruck16:54
clarkbI'm going to try and catch on reviews in various places today as I have to pop out midday for an appointment17:13
clarkbDon't want toget too deep into something I can't put down :)17:13
fungisounds good. i've rolled up my sleeves and am trying to bisect pbr versions first17:14
fungithis should go pretty quickly. pbr 5.7.0 is working and 5.8.0 is not, with everything else the same in the venv (simply downgraded pbr), so i'm pretty sure it's going to turn out to be a pbr regression17:15
fungithere are only 3 changes between them17:16
clarkbinteresting. I wonder if it related to the explicit avoidance of the recursion that the pep XYZ (whatwas the number?) had to do17:17
fungione only changes zuul jobs17:17
clarkbsince that could potentially cahnge what hooks run maybe?17:17
fungiand one only changes tests17:18
clarkbWhat is odd if that is the case was that traces showed that is how it worked when run normally not via the new pep interface. Definitely will be a fun thing to debug if that is the case17:18
fungiso yes, i think it has to be "Allow PEP517 without setup_requires"17:18
fungii'll revert that commit on top of 5.8.0 and see if i get the expected metadata file injected again17:19
fungiokay, not as straightforward as i expected. now upgrading back to pbr 5.8.0 in the venv and things are working17:24
fungii probably have to recreate the venv, may be something to do with editable installs17:24
opendevreviewMerged zuul/zuul-jobs master: ensure-virtualenv: Don't support on CentOS 9-stream
fungiinteresting, updating to pbr with the revert in that venv, things are still broken17:27
fungisuggesting it may not be that change, or that simply reverting it atop master is not sufficient17:28
clarkbneat. I mean when I wrote that chnage I thought I was pretty confident we were preserving the old behavior based on traces of what was going on. So I'm surprised it broke, but I also think that must be the only change that could do it?17:29
fungiyeah, which makes me suspect my test methodology is off17:29
fungineed to simplify it a bit17:29
fungiokay, simpler testing for the win17:33
fungiyes, reverting that change on top of master gets me a gear-0.16.0/gear.egg-info/pbr.json when building gear in a clean checkout with a fresh venv with that modified pbr preinstalled17:34
fungisame as for pbr==5.7.0 preinstalled17:34
fungiwith pbr==5.8.0 preinstalled the pbr.json file is missing from both the sdist and wheel17:34
fungiso i'm confident the regression is there17:35
funginow to revisit how pbr expects to install its metadata file17:35
fungiwrite_pbr_json has a couple of bailouts to short-circuit at start17:38
fungiif not hasattr(cmd.distribution, 'pbr') or not cmd.distribution.pbr17:38
fungiif not git._run_git_functions()17:38
clarkbhrm that first one might be a problem if we want to setting pbr=True but I don't think that has been in the cards yet17:38
fungiif git.get_git_short_sha(git_dir) is not None17:38
fungifailing any of those would cause it not to write the file17:39
fungii can inject some debug prints in there and see what's happening17:39
clarkb*if we want to stop setting pbr=True17:40
*** amoralej is now known as amoralej|off17:41
fungicmd.distribution.pbr is None17:45
clarkbhuh even though pbr=True is set ?17:45
clarkbI wonder if we're sideffecting ourselves then. Because the entire pbr system relies on pbr=True setting that over17:46
clarkbthat is what calls the main pbr entrypoint17:46
fungithat's where it's bailing out of pbr.pbr_json.write_pbr_json()17:46
fungicould it be evaluating a copy?17:46
clarkbthat change sets pbr to None after we are done evaluating it or we thought we were done17:46
clarkbfungi: the reason we were looping is that without the setup_requires entry setuptools kept calling the pbr method because pbr was True17:47
clarkbI didn't fully understand why that was but set it to None to short circuit the recursion17:47
clarkbfungi: I'm thinking that maybe we need to rely on a PBR internal flag rather than setuptools flags once we get out of the main entrypoint17:47
clarkbbasically separate the setuptools state from pbr state with a trnslation layer as necessary in the main entrypoint?17:48
fungii'll need to stare at it a bit more to grok what you're saying17:48
clarkbno worries. I find I have to do that and page a bunch of context in every time too :/17:48
fungithe commit message on the change has a pretty good explanation as to why it's setting it to none17:49
fungiokay, so you're saying we should have pbr itself bail before calling finalize_options if we've already called it once?17:51
fungii wonder what option we can persist that tracking flag in17:52
clarkbfungi: I'm not quite sure of that. We want to call finalize options but then not have setuptools call the pbr entrypoint recursively through that17:52
fungier, what object17:52
clarkbIf we can attach a flag like pbr_in_use to something we can check that rather than distribution.pbr17:52
clarkband continue to set pbr to None at the end of the entrypoint before recursion may occur17:53
fungiyeah, i just need to dig around a bit more to see what objects we can expect to be resident17:53
fungii guess we could try attaching a separate falsey attribute to dist and toggling on its presence rather than its value17:55
fungiadd the attribute on first call, skip if the attribute is already there?17:56
fungiseems dirty though since it's not an object pbr itself instantiates17:56
clarkbya I think distribute/setuptools infer stuff about those attributes too in order to call entrypoints17:57
clarkbI'm not sure if that will lead to errors if the entrypoint isn't present. Or if there ends up being a collision unexpected behavior17:57
clarkbcan we just set a global module flag?17:57
fungiwe could, but is that reentrant?17:58
clarkbI don't think the reentrancy matters here since this flag would flip from false to True once and then never change17:58
fungieven if something else imports pbr, yeah i guess the import is a noop so long as it's not a reload17:59
clarkbfungi: Don't change the check for pbr and flipping it to None in the entrypoint. Instead set pbr.core.in_use to True at the beginning of the entrypoint and then have the json writer check that value instead of the dist value17:59
*** akahat|dinner is now known as akahat|rover17:59
fungi"the entrypoint" being
clarkbfungi: pbr.core.pbr18:01
clarkbI believe that is the top level entrypoint into pbr18:01
fungioh, that entrypoint, okay18:01
clarkbsince setuptools looks for a method called pbr when you set pbr=True in its setup() arglist18:01
fungiyeah, i thought you meant the pep 417 build backend entrypoint18:01
clarkband in that we set pbr to None to prevent recursion. Which causes the json writing to noop18:02
clarkbbut I think we can address that by having the json writer check for a different value and not modify pbr() setting of dist.pbr to None18:02
clarkbBecause if we don't stop setting dist.pbr to None it does infinite recursion through pep 517 without setup_requires18:02
clarkb(I don't undersatnd why, we could potentially debug that behavior morefully and come up with a better answer, but I spent a bit of time trying to undersatnd that before giving up)18:03
fungii guess i need to declare in_use as a global in pbr.core?18:10
clarkbI think you do when accessing it in a method otherwise it assumes scope local if not scoped18:10
fungiit's not really a method so much as a function in this case, but yeah trying18:11
fungii'll strip out the debug prints, add some code comments, and see if i can come up with a regression test18:12
fungii'll also test it in a pep-517 using project (i've been testing with gear so far)18:13
fungimmm, pep-517 build isolation makes this harder18:22
fungito test18:22
fungithe installed pbr doesn't get used18:22
fungitrying to set the path to my modified pbr in pyproject.toml instead18:24
clarkbfungi: in the existing tests for pep517 we use build beacuse you can turn off isolation18:24
clarkbfungi: that allows us to preinstall what we want and test without being overriddent from pypi18:25
fungiyeah, but i want to make sure isolation doesn't break pbr18:25
fungiwriting pbr to mudpy.egg-info/pbr.json18:25
fungiyep, working!18:26
fungipbr.json is showing up in both the sdist and wheel18:27
fungiokay, so just need to comment this up and figure out how to test it18:27
fungiin zuul18:27
clarkbfungi:  Ithink you should be able to take the existing tests that install a package and inspect the metadata pbr.json for sha content18:28
clarkbshouldn't need new tests, just update a coupel of the existing tests to ensure that data is making it into the final product?18:28
fungiyeah, that could work. thanks18:30
fungii'll squash the test into the fix once zuul confirms both are working18:41
clarkbfungi: left a note on the fix change18:43
clarkba small simplification18:43
fungiclarkb: yeah, i originally had it initialize to false but then realized we had the hasattr check making it unnecessary. potato/potato i guess18:46
fungimore pythonic would be to just check the attr directly and then catch the exception raised when it's absent18:47
fungibut pbr has a lot of lbyl style so i tried to mimic what's there18:47
clarkbfor flags we know we always want to check I alawys like to set them up18:52
fungiyeah, if you prefer it that way, i'm happy to initialize it first18:55
clarkbI'm fine either way. Thank you for looking into this and fixing it18:56
fungiwell, it seems relatively urgent, because we're publishing releases with no git metadata in them at the moment18:56
fungionce we're good with the fix and regression test, i'll see if we can expedite a release18:56
fungihuh, the fix isn't fixing the test (aside from the fact that the test is checking for the wrong list item. the test package doesn't seem to include a pbr.json19:10
clarkbI wonder if it is using the wrong version of pbr because setup_requires19:11
clarkbfungi: I think that might be happening19:12
fungiso the other tests there are also bunk in that case19:13
clarkbif you look in test_packaging there is much more robust virtualenv setup and installtion of pbr to the code under test19:13
clarkbbut the test you modified is relying on the base test class which seems far more rudimentary19:13
fungiyeah, i have a feeling that existing test was useless19:14
fungiokay, managed to get that working locally19:34
clarkbI went ahead and +2'd both changes since I'm not sure I'll be around when CI results come back. I think landing that quickly is worthwhile19:39
clarkbfeel free to reapply my +2 postsquash19:39
clarkbfungi: looks like unittests are still failing?19:48
clarkbyou may need to instrument those cases and check that pbr is what you expect it to be?19:48
fungihuh, yeah looks like it's failing pbr.tests.test_pbr_json.TestJsonContent.test_content because write_file wasn't called?19:57
Clark[m]I strongly suspect that it just isn't getting an updated pbr. Maybe yolo some print statements in there to see19:58
Clark[m]In pbr() if you print it should show up if new pbr is used19:58
fungiwell, that's not the test i added, it's an existing test19:58
fungiso presumably my change broke it19:58
Clark[m]Oh I see19:59
fungireprodicible locally too, so i'll see if i can work out what's gone wrong20:00
fungithe test is calling directly into pbr_json.write_pbr_json() and so our global flag from the entrypoint is never set20:03
fungii guess the test could set that flag, i'll try it20:03
fungiyep, that'll work20:05
fungiwell, it's passing more jobs now, at least. not all20:31
fungi looks like some sort of transitive import problem specific to python 2.720:33
fungirecursive import maybe?20:34
Clark[m]Ya looks like it is a loop that it is complaining about20:36
fungithe pbr_json->core is what my patch added20:36
fungipy3x seems to take it in stride20:36
Clark[m]Maybe we can put the flag in to avoid that?20:37
Clark[m]Hrm maybe not20:37
Clark[m]But I'm no longer at a computer so hard to think through it20:37
fungiyeah, i'll noodle on it20:37
fungiif we put it in util then we still have the cycle, it just closes one import sooner20:38
fungisince pbr_json will need to import util directly instead of importing core20:38
fungilooks like the problem might be specific to the "from foo import bar" construct20:40
fungiokay, if i replace the "from pbr import util" in pbr.core and switch its util calls to pbr.util then the circular import problem does seem to go away20:43
fungii'll propose that as a separate patch just to make sure we're not papering over anything in the other one20:45
fungihrm, no, pbr.core defines a pbr function locally, which ends up shadowing the pbr module. this is a mess21:20
fungier, shadowing the pbr package21:21
fungithis will need more thought21:21
* fungi already despised relative imports, but now hates them even more21:21
Clark[m]I wonder if we can just assume if that method is called that pbr is in use21:21
Clark[m]Drop the flag entirely I mean21:21
fungiworth a try21:22
fungii'll rejigger all this and see what i get21:22
Clark[m]Off the top of my head I'm not sure what context it would be called in where we don't want the file to be generated21:22
fungiright, it was probably a safeguard against it being called when there's no dist to process21:34
clarkbhrm like when doing a othercommand?21:38
fungii doubt we'd actually call into the module in those situations, but i guess testing will prove whether it's safe21:38
clarkbseems reasonable21:39
clarkbfungi: I suspect that it may be because pbr.json is setup as an egg info writer entrypoint21:42
clarkbso if you've got pbr installed but are doing a non pbr package you need it to noop?21:42
clarkbThis raelly is a mess :(21:42
fungioh, yeah so for building non-pbr packages when pbr is installed in the environment21:43
clarkband that explains why this broke in the first place since it doesn't get invoked via the pbr() entrypoint21:43
clarkbits a separate entrypoint after pbr is processed21:43
fungiso we need some way to identify whether the dist for which this is being called is a "pbr" type dist21:44
clarkbfungi: maybe we can get away with hasattr pbr21:44
fungiand we can't rely on the presence of the pbr entrypoint for the dist because we need to turn it off after21:44
fungioh, right!21:44
clarkbsince I believe that is only set if you put a pbr=Something flag in a setup() call21:45
clarkbI suppose you may set it to False or None explicitly though and then that gets weird21:45
clarkbattrs = util.cfg_to_args(path, dist.script_args) <- that is the only thing in util that pbr.core.pbr needs21:47
clarkbWe might be able to split that into its own separate thing to break the loop21:48
clarkbhrm but that ends up calling a bunch of other stuff in util21:49
clarkbAnother approach would be to track entries into pbr() and return if >121:50
clarkbthen leave dist.pbr = True21:50
fungioh, like have core exit with extreme prejudice if the in_use global is true?21:51
fungii can try that next21:51
clarkbya basically only check the in_use flag in core.pbr() and then avoid any import trouble by having the tendril code look at dist.pbr21:52
clarkband remove the switch to None. That mgith end up being the cleanest thing21:52
clarkbAs ugly as it is it preserves the existing check we knew worked. And should avoid the recursion that we're trying to avoid21:53
clarkbfungi: I just pushed the counter idea if you want to look at it and see if that seems good. Feel free to edit or replace too21:58
fungii also pushed the one we were just discussing to see if it will work21:59
fungilocal spot checks of some tests i was worried about came out clean at least21:59
fungii need to context-switch to dinnermaking now, but will check on things once i'm done eating21:59
clarkbya yours is basically what i did. I think it is worth writing down in the new comments where the source of recursion is happening. I tried to do that in my change but I'm hapyp with either one or to have you update your change to update the comments22:00
clarkband ya thinking about it maybe a binary flag is better than a coutner since we only care if not 0 count22:00
clarkbI'll leave a note about improving comments on yours. enjoy dinner! 22:01
*** dviroel|ruck is now known as dviroel|out22:06
clarkbfungi: testing on the recursion fixup change looks really good. I think that is the one to go with. Just need to update the comment, squash into the test and then merge?23:14
opendevreviewClark Boylan proposed opendev/system-config master: Install Limnoria from upstream
clarkbmaybe next week I'll actually find time to land that when meetings aren't happenign and I can double check stuff23:22
clarkbcool got a +1 so ya I think we can add more text to the comment and then squash in the test change and send it23:29
fungiyeah, working on it now23:33
fungiclarkb: updated, thanks!23:36
clarkbI went ahead and approved it. Thanks. I think others can chime in between now and whenever a release is made if they want changes made23:40

Generated by 2.17.3 by Marius Gedminas - find it at!