New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

P4 with python 3.6 under windows fails on worker #3493

Closed
sphet opened this Issue Aug 3, 2017 · 18 comments

Comments

Projects
None yet
5 participants
@sphet
Contributor

sphet commented Aug 3, 2017

Trying to migrate from python 2.7 to python 3.6 on worker and getting following error in Perforce sync command:

'b'p4'' is not recognized as an internal or external command,
operable program or batch file.

Which is interesting because the command is

b'p4' b'-p' b'192.168.1.10:4000' b'-u' b'ABC' b'-c' b'ABC-buildbot09-build-targets' b'client' b'-i'
in dir D:\buildbot\build-targets\build (timeout 1200 secs)
watching logfiles {}
argv: [b"b'p4'", b"b'-p'", b"b'192.168.1.10:4000'", b"b'-u'", b"b'ABC'", b"b'-c'", b"b'ABC-buildbot09-build-targets'", b"b'client'", b"b'-i'"]
writing 547 bytes to stdin
using PTY: False

Which has doubly-quoted binary items here.

Not sure if the problem is the binary string or something else. Master is running 3.4.3

@sphet

This comment has been minimized.

Show comment
Hide comment
@sphet

sphet Aug 3, 2017

Contributor

I found that if I modified line 245 in buildbot/p4.py which reads:

command = [encodeArg(c) for c in command]

and simply don't encode each argument the problem is fixed.

However, I suspect this would break python 2.7.

Any ideas?

Contributor

sphet commented Aug 3, 2017

I found that if I modified line 245 in buildbot/p4.py which reads:

command = [encodeArg(c) for c in command]

and simply don't encode each argument the problem is fixed.

However, I suspect this would break python 2.7.

Any ideas?

@rodrigc

This comment has been minimized.

Show comment
Hide comment
@rodrigc

rodrigc Aug 4, 2017

Collaborator

I think your patch is on the right track.

Can you try your patch in python 2 and python 3 setups and verify if it works with perforce?
I don't have an actual p4 environment to test against.

Alternatively, if you can give me some quick steps I can do to set up p4 to test, I can try that.

Collaborator

rodrigc commented Aug 4, 2017

I think your patch is on the right track.

Can you try your patch in python 2 and python 3 setups and verify if it works with perforce?
I don't have an actual p4 environment to test against.

Alternatively, if you can give me some quick steps I can do to set up p4 to test, I can try that.

@seankelly

This comment has been minimized.

Show comment
Hide comment
@seankelly

seankelly Aug 4, 2017

Member

I think encodeArg isn't doing it right then. I last touched that line, but it was there before so I don't know the original reason.

Member

seankelly commented Aug 4, 2017

I think encodeArg isn't doing it right then. I last touched that line, but it was there before so I don't know the original reason.

@sphet

This comment has been minimized.

Show comment
Hide comment
@sphet

sphet Aug 4, 2017

Contributor

@seankelly I am no python expert but I wonder if the encodeArg is there to handle unicode strings in some of the client names, etc. In 2.7 it would need to be encoded as ascii, but in 3.x the encode is making it a byte-string which I think needs to be decoded as ascii. Other commands in p4 in the same file have no such encoding.

@rodrigc I don't have python 2 setup here anymore - but I think it would be moot test for me - we don't have utf-8/multibyte client or machine names here since we are in North America. The concern would be more with langues that use unicode to handle accents or non-latin text.

Maybe someone who knows more about this can decide what to do -- I suspect we need to decode the resulting byte string into ascii but I have not use 2.7 for a looonnng time.

Contributor

sphet commented Aug 4, 2017

@seankelly I am no python expert but I wonder if the encodeArg is there to handle unicode strings in some of the client names, etc. In 2.7 it would need to be encoded as ascii, but in 3.x the encode is making it a byte-string which I think needs to be decoded as ascii. Other commands in p4 in the same file have no such encoding.

@rodrigc I don't have python 2 setup here anymore - but I think it would be moot test for me - we don't have utf-8/multibyte client or machine names here since we are in North America. The concern would be more with langues that use unicode to handle accents or non-latin text.

Maybe someone who knows more about this can decide what to do -- I suspect we need to decode the resulting byte string into ascii but I have not use 2.7 for a looonnng time.

@seankelly

This comment has been minimized.

Show comment
Hide comment
@seankelly

seankelly Aug 4, 2017

Member

I am wondering if encodeArg is needed. The rest of the source steps aren't doing the same thing. GitPoller, SVNPoller, and P4Poller all use encodeArg/encode, but HgPoller doesn't.

Member

seankelly commented Aug 4, 2017

I am wondering if encodeArg is needed. The rest of the source steps aren't doing the same thing. GitPoller, SVNPoller, and P4Poller all use encodeArg/encode, but HgPoller doesn't.

@seankelly

This comment has been minimized.

Show comment
Hide comment
@seankelly

seankelly Aug 5, 2017

Member

Are you using P4Poller to monitor changes too?

Member

seankelly commented Aug 5, 2017

Are you using P4Poller to monitor changes too?

@sphet

This comment has been minimized.

Show comment
Hide comment
@sphet

sphet Aug 5, 2017

Contributor
Contributor

sphet commented Aug 5, 2017

@rodrigc

This comment has been minimized.

Show comment
Hide comment
@rodrigc

rodrigc Aug 16, 2017

Collaborator

@asomers can you provide me with a very small recipe for creating a Perforce repository which can be polled with buildbot with this: http://docs.buildbot.net/latest/manual/cfg-buildsteps.html#p4

I want to try to fix some of the issues preventing this from working with Python 3

Collaborator

rodrigc commented Aug 16, 2017

@asomers can you provide me with a very small recipe for creating a Perforce repository which can be polled with buildbot with this: http://docs.buildbot.net/latest/manual/cfg-buildsteps.html#p4

I want to try to fix some of the issues preventing this from working with Python 3

@bdbaddog

This comment has been minimized.

Show comment
Hide comment
@bdbaddog

bdbaddog Aug 16, 2017

Member

Wow.. I went to look at perforce docs (which used to be well organized) and they've degraded significantly. I did p4 admin work a while back.

Member

bdbaddog commented Aug 16, 2017

Wow.. I went to look at perforce docs (which used to be well organized) and they've degraded significantly. I did p4 admin work a while back.

@bdbaddog

This comment has been minimized.

Show comment
Hide comment
@seankelly

This comment has been minimized.

Show comment
Hide comment
@seankelly

seankelly Aug 17, 2017

Member

I've been working on this off-and-on and have something that can create a repository that Buildbot can poll. This is what I have in a Dockerfile:

/opt/perforce/sbin/configure-helix-p4d.sh master -n -p 1666 -r /srv/perforce/master -u super -P SuperSuper
echo 'SuperSuper' | p4 login
p4 configure set security=0
p4 depot -i < /root/perforce/test-depot
p4 client -i < /root/perforce/client
mkdir -p /root/work/test
cd /root/work/test
echo 00 > 00
P4CLIENT=buildbot_test p4 add 00
P4CLIENT=buildbot_test p4 submit -i < /root/perforce/submit
p4dctl stop -a

The redirected files aren't important. So long as an editor is set, Perforce will open something usable.

Member

seankelly commented Aug 17, 2017

I've been working on this off-and-on and have something that can create a repository that Buildbot can poll. This is what I have in a Dockerfile:

/opt/perforce/sbin/configure-helix-p4d.sh master -n -p 1666 -r /srv/perforce/master -u super -P SuperSuper
echo 'SuperSuper' | p4 login
p4 configure set security=0
p4 depot -i < /root/perforce/test-depot
p4 client -i < /root/perforce/client
mkdir -p /root/work/test
cd /root/work/test
echo 00 > 00
P4CLIENT=buildbot_test p4 add 00
P4CLIENT=buildbot_test p4 submit -i < /root/perforce/submit
p4dctl stop -a

The redirected files aren't important. So long as an editor is set, Perforce will open something usable.

@asomers

This comment has been minimized.

Show comment
Hide comment
@asomers

asomers Aug 17, 2017

Contributor

@rodrigc I'm travelling right now, but I can give it a shot next Thursday if you haven't solved it yet.

Contributor

asomers commented Aug 17, 2017

@rodrigc I'm travelling right now, but I can give it a shot next Thursday if you haven't solved it yet.

@sphet

This comment has been minimized.

Show comment
Hide comment
@sphet

sphet Aug 17, 2017

Contributor

Thanks for following up on this.

Contributor

sphet commented Aug 17, 2017

Thanks for following up on this.

@rodrigc

This comment has been minimized.

Show comment
Hide comment
@rodrigc

rodrigc Aug 18, 2017

Collaborator

I created a perforce repository by doing:

export P4HOST=localhost
export P4PORT=1666
mkdir /tmp/sample-p4-repo
p4d -r /tmp/sample-p4-repo

On client, I was able to do:

export P4HOST=localhost
export P4PORT=1666
p4 info

In master.cfg, I was able to add:

p4source = changes.P4Source(p4base='//depot/project/', 
                                                     p4port="localhost:1666",
                                                     pollInterval=15, pollAtLaunch=True,
                                                     split_file=lambda branchfile: branchfile.split('/',1))
c['change_source'].append(p4source)

factory.addStep(steps.P4(p4port='localhost:1666',
                         p4client=util.WithProperties('localhost'),
                         p4user='crodrigues',
                         p4base='//depot',
                         mode='incremental'))

What else do I need in master.cfg to get this to work?

Collaborator

rodrigc commented Aug 18, 2017

I created a perforce repository by doing:

export P4HOST=localhost
export P4PORT=1666
mkdir /tmp/sample-p4-repo
p4d -r /tmp/sample-p4-repo

On client, I was able to do:

export P4HOST=localhost
export P4PORT=1666
p4 info

In master.cfg, I was able to add:

p4source = changes.P4Source(p4base='//depot/project/', 
                                                     p4port="localhost:1666",
                                                     pollInterval=15, pollAtLaunch=True,
                                                     split_file=lambda branchfile: branchfile.split('/',1))
c['change_source'].append(p4source)

factory.addStep(steps.P4(p4port='localhost:1666',
                         p4client=util.WithProperties('localhost'),
                         p4user='crodrigues',
                         p4base='//depot',
                         mode='incremental'))

What else do I need in master.cfg to get this to work?

@sphet

This comment has been minimized.

Show comment
Hide comment
@sphet

sphet Aug 18, 2017

Contributor
Contributor

sphet commented Aug 18, 2017

@rodrigc

This comment has been minimized.

Show comment
Hide comment
@rodrigc

rodrigc Aug 18, 2017

Collaborator

OK, I added a steps.P4 to my master.cfg, and I have a working setp where I can reproduce your problem.

Collaborator

rodrigc commented Aug 18, 2017

OK, I added a steps.P4 to my master.cfg, and I have a working setp where I can reproduce your problem.

@rodrigc

This comment has been minimized.

Show comment
Hide comment
@rodrigc

rodrigc Aug 18, 2017

Collaborator

I see the problem. One place where the double encoding is here:
https://github.com/buildbot/buildbot/blob/v0.9.9/worker/buildbot_worker/util/__init__.py#L75

Collaborator

rodrigc commented Aug 18, 2017

I see the problem. One place where the double encoding is here:
https://github.com/buildbot/buildbot/blob/v0.9.9/worker/buildbot_worker/util/__init__.py#L75

@rodrigc

This comment has been minimized.

Show comment
Hide comment
@rodrigc

rodrigc Aug 18, 2017

Collaborator
Collaborator

rodrigc commented Aug 18, 2017

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