Skip to content

Perforce bytes fix#3543

Merged
tardyp merged 4 commits intobuildbot:masterfrom
rodrigc:perforce-bytes-fix
Aug 21, 2017
Merged

Perforce bytes fix#3543
tardyp merged 4 commits intobuildbot:masterfrom
rodrigc:perforce-bytes-fix

Conversation

@rodrigc
Copy link
Copy Markdown
Contributor

@rodrigc rodrigc commented Aug 18, 2017

Fix a case where the Perforce step was passing bytes all the way down
to RemoteCommand. buildbot_worker.util.__init__.Obfuscated(b"something") was returning a string "b'something'" instead of b"something" which was causing things to break.

Fixes #3493

Importing from future.builtins to override str and bytes
actually leads to some subtle problems on Python 2.
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 18, 2017

Codecov Report

Merging #3543 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3543      +/-   ##
==========================================
- Coverage   88.27%   88.27%   -0.01%     
==========================================
  Files         323      323              
  Lines       33781    33779       -2     
==========================================
- Hits        29819    29817       -2     
  Misses       3962     3962
Impacted Files Coverage Δ
master/buildbot/process/remotecommand.py 83.47% <100%> (ø) ⬆️
worker/buildbot_worker/util/__init__.py 96.72% <100%> (ø) ⬆️
master/buildbot/steps/master.py 92.35% <100%> (-0.1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a96902...b9591fe. Read the comment docs.

@rodrigc
Copy link
Copy Markdown
Contributor Author

rodrigc commented Aug 18, 2017

@sphet Please try this patch which fixes #3493.
Please apply the patch manually, or obtain the whole tree from git by doing:

git clone -b perforce-bytes-fix https://github.com/rodrigc/buildbot

Please report back.

@sphet
Copy link
Copy Markdown
Contributor

sphet commented Aug 19, 2017

Thanks @rodrigc - I will be back in the office on Wednesday PDT and will check it then.

@seankelly
Copy link
Copy Markdown
Member

This fixes the issue for me.

@rodrigc
Copy link
Copy Markdown
Contributor Author

rodrigc commented Aug 19, 2017

Could you describe your setup, configuration, and what steps you followed to verify the fix?

@seankelly
Copy link
Copy Markdown
Member

The setup. It can replicate the problem.

I installed your branch for master and worker on Python 2 and 3. After that, it works as expected.

@rodrigc
Copy link
Copy Markdown
Contributor Author

rodrigc commented Aug 19, 2017

Thanks for providing details about your test setup, and testing on Python 2 and 3!

@tardyp tardyp closed this Aug 21, 2017
@tardyp tardyp reopened this Aug 21, 2017
@tardyp tardyp merged commit 4c6e4b5 into buildbot:master Aug 21, 2017
@sphet
Copy link
Copy Markdown
Contributor

sphet commented Aug 30, 2017

Does this change require upgrading all the workers as well, or only the master?

I can report that upgrading the master and the worker to the head (0.9.11-dev143) solves this problem. Thank you very much.

@rodrigc
Copy link
Copy Markdown
Contributor Author

rodrigc commented Aug 30, 2017

You need to upgrade both

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants