https://pantsbuild.org/ logo
Join Slack
Powered by
hm... the only non-cron run of master had all of t...
# announce
w
hm... the only non-cron run of master had all of the wheels shards fail in the same way. i've restarted them, but am not optimistic. https://travis-ci.org/pantsbuild/pants/builds/507685801
h
What was the failure?
w
didn't save it... a missing directory error.
👍 1
h
Something to do with 
.pants-interpreter-constraints
file?
w
no
👍 1
a
i knew it
h
Ughhhh why, the 
ENV
is being set properly! https://travis-ci.org/pantsbuild/pants/jobs/507685813#L519
a
ok at least it's failing reliably
👍 1
w
yea, failed the same way on the first attempt
i'm preeetty close to cutting 
1.15.x
(ideally today), so if we don't think there is a tiny fix-forward, a revert might be the way to go
h
Wait @aloof-angle-91616, does it need an 
=
in that 
ENV
line? We use 
=
every other 
ENV
line
a
the 
ENV
line accepts two formats
👍 1
i like to use the one with a space because it makes it clear that only one variable is getting set? or something? but it should work, otherwise e.g. pyenv in #7064 wouldn't work i think
i will spend a bit finding a tiny fix-forward but as this is entirely internal development-facing and does not affect any consumer of 
1.15.x
i would strongly bias towards a revert
i have absolutely no clue why this wouldn't show up in CI on the PR, which was green 4 times in a row before merging
👍 1
my other PR e.g. #7064 isn't failing
w
the wheel builder shards upload to S3 when run on master... might be something inside of that conditional logic failing
🤔 1
a
that sounds like a very good thing to check
buuuuuut that would only be the one wheel builder shard like the py 3.6 engine build shouldn't be doing that
wait
i'm wrong that's about 
fs_util
👍 1
w
there are 6 wheel builder shards
they all have the same error
a
it's almost definitely because of uploading that to s3 though i think now
h
Yes that makes sense to me. Sounds like Docker env vars and Travis global env vars don’t propagate to S3's environment?
a
death
😢 1
i don't see where we upload wheels to s3? i can see where we pull them down here: https://github.com/pantsbuild/pants/blob/067238335e8bbe2dacbe6442548dd4ee5a0782df/build-support/bin/release.sh#L456, but i don't see where we're uploading things to s3
i can also run these locally
running these locally involves building rust in a docker container, so am waiting on that but will wait for a bit
👍 1
my recommendation is probably to revert this now since i am positive this change is what is breaking master and then to try to repro the error and unrevert later today
w
works for me.
a
i can make the revert diff
w
the signal for S3 uploads is setting 
PREPARE_DEPLOY
a
ah
w
sorry. but yea, revert would be good.
(or if you want to look at it a bit longer, that's fine too)
a
i'm gonna spend 5-10 more minutes then put up a revert
w
if we can have one or the other fix reviewable by mid-afternoon, i'm golden
❤️ 1
a
i have a local repro
🎉 1
this is...a weird error
the failing command is 
find /source/dist -type f -name 'pantsbuild.pants-1.15.0.dev4+06723833-*.whl'
the wheel it's looking for is 
dist/pantsbuild.pants-1.15.0.dev4+06723833/dist/pantsbuild.pants-1.15.0.dev4+6723833-cp27-cp27mu-linux_x86_64.whl
the wheel filename uses 
+6723833
without a leading 0
🙃 1
h
Huh that’s expected to be there.. When you revert the commit, it no longer fails on that line?
a
if so, that would just be because the commit sha is different and doesn't have a leading 0
h
So do you think this might actually not even be related to auto clean-all, and it’s a bug we’ve had all along that we’re only now hitting?
a
yes because i've seen it before
👍 1
i'm investigating why the leading 0 is stripped
w
hah. good find.
a
and to make the context more clear, i was able to reproduce the CI failure locally using docker, and i was able to know that that was the failure by adding 
-d
to the 
release.sh
command line, which showed the failing 
find
command
now i suspect the 
setup-py
task
😕 1
w
@aloof-angle-91616: so, is your suspicion then that this will not repro for another commit unless it has a leading 
0
?
a
yes
w
if so, i might proceed with the release.
k
thanks a lot for investigating.
a
if i could ask you to do the revert that would be helpful since i have my pants repo tied up in investigating this now
i'll sign off on it
w
@aloof-angle-91616: do you think that we need the revert?
a
oh, no
w
👍
a
yeah this is python's fault
👍 1
Copy code
X> pex packaging setuptools
Python 2.7.15 (default, Jun 20 2018, 17:40:09)
[GCC 4.2.1 Compatible Apple LLVM 9.0.0 (clang-900.0.39.2)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
(InteractiveConsole)
>>> from packaging.version import Version
>>> Version('1.15.0.dev4+06723833')
<Version('1.15.0.dev4+6723833')>
>>> str(Version('1.15.0.dev4+06723833'))
'1.15.0.dev4+6723833'
>>>
😢 1
the PEP doesn't say anything about that
oops
it does
Comparison and ordering of local versions considers each segment of the local version (divided by a .) separately. If a segment consists entirely of ASCII digits then that section should be considered an integer for comparison purposes and if a segment contains any ASCII letters then that segment is compared lexicographically with case insensitivity. When comparing a numeric and lexicographic segment, the numeric section always compares as greater than the lexicographic segment. Additionally a local version with a great number of segments will always compare as greater than a local version with fewer segments, as long as the shorter local version's segments match the beginning of the longer local version's segments exactly.
hm
it doesn't say it kills the 0
just "for comparison purposes"
so this isn't documented at least in there, googling to see if there's an obvious fix
h
Yeah that’s concerning. An int in Python strips leading zeros. We would expect it to keep the leading zero when calling str()
a
the issue is that pex is the one calling 
str(packaging.version.Version(version))
so we would need to edit the version string i think
w
the format of those hashes isn't really specified anywhere... so we could use a different format
a
yeah
this i think really deserves an upstream fix
not important right now
w
in fact, DWH was recently saying that he would love for them to be time ordered...
❤️ 1
a
intellij and scala projects do a thing with the timestamp and commit sha, but they don't have python mangling the versions
(we should definitely use the scala thing, i'll find an example)
maybe it's an sbt thing specifically
h
+1 for time ordered. I’ve wasted so much time trying to figure out which 
dist/
entry I want when debugging
a
does your 
ls
not sort by modified time? loser
actually, mine doesn't either
😛 1
h
FYI if we keep the current hash scheme, we can fix this with 
.zfill(10)
(or however many chars hash should be)
Copy code
"123".zfill(4) -> "0123"
"1234".zfill(4) -> "1234"
a
"collisions"
i was thinking replacing zero with something that's not a hex char
perhaps 
Z
the problem is that if you give the correct version, 
packaging.version.Version()
will distort it -- i would like to have a version that isn't changed rather than to work around broken behavior
i'm going to try using 
Z
instead of 
0
and seeing if that works
no clue how to test this but that should fix it for now
replacing 
0
with 
Z
seems to work, taking out the trash while it goes
lol, it also gets lowercased during normalization, which makes sense, using a lowercase 
z
instead
w
I don't think you need to replace anything, just prefix with non-numerical, right?
a
yes
w
(or numerical that is guaranteed not to have a leading zero)
a
trying that
i'm prefixing it with 
git
and if this works going to push that and leave a TODO with an issue to use a version that is ordered by timestamp too like sbt does
when this script completes i will push a diff with the (small!) fix
back to work
2 Views
Open in Slack
PreviousNext
Powered by