harcoded tmp folder prevent windows compatibilty of past module #296

kapilkd13 · 2017-08-07T19:13:27Z

For issue #295
Here we are using temp env variable to figure out right directory to write files like original_code.py in past's translate.
I am working on a project that rely on past module and currently before calling the past module's function I am creating Temp directory as a workaround. Once this PR is merged I don't have to use that workaround.
It would be good to make this module independent of any OS. Thanks

mr-c · 2017-08-08T14:06:04Z

src/past/translation/__init__.py

@@ -219,20 +220,20 @@ def detect_python2(source, pathname):
    if source != str(tree)[:-1]:   # remove added newline
        # The above fixers made changes, so we conclude it's Python 2 code
        logger.debug('Detected Python 2 code: {0}'.format(pathname))
-        with open('/tmp/original_code.py', 'w') as f:
+        with open(os.path.join(temppath,'original_code.py'), 'w') as f:


I recommend using tempfile.mkdtemp instead of this and removing line 211 (temppath)

Also fixes bug related to simultaneous use of past on the same system

@mr-c Thanks for the suggestion. I have made appropriate changes.
And Simultaneous use of past module is an interesting use case. Never thought about it. 👍

mr-c

LGTM!

serge2016 · 2017-10-10T07:00:17Z

It's great!
This also will help when this module is used with different users on the same OS! In this case first user automatically and unknownly creates files futurized_code.py, original_code.py and py2_detection_code.py in the temp folder /tmp and blocks them for rewriting for other users!
So this PR fixes this behaviour. But I think it also would be great if these 3 files are automatically deleted in the end of execution!

mr-c · 2017-10-10T09:07:00Z

@serge2016 Thanks. Can we get this PR merged ASAP so we can remove our workaround?

kapilkd13 · 2017-10-10T09:17:21Z

@serge2016 I agree that we should remove the temp file once our work is done. I can make a separate PR for it. In the meanwhile once this PR get merge we can remove our workaround as mentioned by @mr-c

serge2016 · 2017-10-11T05:15:50Z

@kapilkd13, @mr-c How can I help you?

kapilkd13 · 2017-10-11T05:18:39Z

@serge2016 Actually we were hoping that you have commit access to this repo. My bad :)

serge2016 · 2017-10-11T05:21:39Z

@kapilkd13 Any suggestions what do I have to do to speed up merging of this PR?
Maybe it would be better to add the deletion of these files to this PR?

serge2016 · 2017-11-09T04:20:48Z

@edschofield, @QuLogic, @krischer, dear colleagues! Could you, please, check this PR and merge it?

krischer · 2017-11-09T07:25:14Z

I'm just a user of this library - I have no rights to the repository and also not enough knowledge of the library's internals to judge this.

serge2016 · 2017-11-09T07:30:28Z

@krischer you are a contributor)
Then whom do we have to ask for merge?

serge2016 · 2017-12-07T04:55:44Z

@edschofield, could you help us, please, to get this PR merged?

d1skort · 2017-12-27T12:14:56Z

Any change to merge?

theferrit32 · 2018-01-11T19:09:42Z

Is there something blocking the merging of this PR? If not, can it be merged into master as soon as possible?

This fixes an additional problem I am encountering. When past.translation is used from python3.x by two different users on the same machine, the second user will be unable to do so due to the permissions of the files created in the top level directory of /tmp. The second user gets a PermissionError because those files already exist and are owned by the first user, and the error is unresolvable unless the second user has admin rights on the machine and can/does wipe out certain contents of /tmp manually, which is very undesirable for me.

Writing those files to a subdirectory within /tmp instead of the top level directory solves it like in this PR.

d1skort · 2018-03-26T10:00:22Z

Guys please any change to merge?

ADKosm · 2018-03-26T10:06:01Z

PR looks good. Maybe it's time to merge it? @mr-c

mr-c · 2018-03-26T10:44:11Z

@ADKosm Fine by me, but I don't have permissions here

serge2016 · 2018-03-26T10:45:43Z

@mr-c Maybe you know, who does?

mr-c · 2018-03-26T10:51:57Z

@serge2016 I have no relationship with this project. Looks like @edschofield merged the last round of accepted PRs

kapilkd13 · 2018-04-01T10:56:23Z

@mr-c It looks like no one is managing this repo now. Is there a way to gain commit access here.

mr-c · 2018-04-01T11:05:55Z

@kapilkd13 Someone (or better, a group) could fork this, rename it and maintain it themselves. There is no way to gain commit access to a repository without assistance from the repository owner.

kapilkd13 · 2018-04-01T11:14:44Z

@mr-c A new maintainer group with fork of this repo sounds good.
Also, I have seen a lot of awesome repositories that no one is maintaining now. Wouldn't it be good idea if there is a group or organization of willing people who would maintain such repos and once a developer feels he can no longer devote time to their repo he can transfer access rights to this organization. Just thinking 😸

bakkerthehacker · 2018-04-09T22:29:31Z

src/past/translation/__init__.py

@@ -395,7 +396,7 @@ def load_module(self, fullname):

                        if detect_python2(source, self.pathname):
                            source = self.transform(source)
-                            with open('/tmp/futurized_code.py', 'w') as f:
+                            with open(os.path.join(tempfile.mkdtemp(),'futurized_code.py'), 'w') as f:


This doesnt actually work. The existing code assumes that the futurized_code.py is in the same path as above.

+1 -- it might make sense to call tempfile.mkdtemp() once during test setup, refer to it throughout, and delete on cleanup.

theferrit32 · 2018-04-10T18:48:16Z

@bakkerthehacker is right, this changeset will still (silently) not clean up the files (the previous code didn't either), though it will fix the file permission issues and the windows compatibility by using tempfile.mkdtemp for each execution instead of /tmp.

If those 3 files being written are unused by the package code, it is even better just to not write them in the first place. Doing a search for the filenames doesn't show any reference to them after writing. PR #335 removes them, and can resolve this PR.

The last merge was completed 11 months ago. I think if there is still no reply within some time frame going forwards, we will need to think about where a fork of this code will need to live, and have a support request submitted for the package in PyPI in order to give it a new set of maintainers.

serge2016 · 2018-09-12T04:50:56Z

Hello! Any chances to get progress here?

kapilkd13 · 2018-09-12T05:36:07Z

@jmadler I saw you made a recent commit, can you merge this PR.

jmadler · 2018-09-17T22:47:06Z

src/past/translation/__init__.py

@@ -395,7 +396,7 @@ def load_module(self, fullname):

                        if detect_python2(source, self.pathname):
                            source = self.transform(source)
-                            with open('/tmp/futurized_code.py', 'w') as f:
+                            with open(os.path.join(tempfile.mkdtemp(),'futurized_code.py'), 'w') as f:


+1 -- it might make sense to call tempfile.mkdtemp() once during test setup, refer to it throughout, and delete on cleanup.

jmadler · 2018-09-17T22:50:57Z

@kapilkd13 one minor change but otherwise looks good.

Would you be open to setting up a PR to support Windows more broadly? Getting travis to run on Windows and all tests to succeed would be a good first step. You can build on the matrix config from #366 once that's landed.

theferrit32 · 2018-09-17T23:03:04Z

@jmadler please look at #335 first which just stops the code from writing these files to begin with. If that is accepted then it obsoletes this pull request.

jmadler · 2019-05-06T20:07:48Z

SGTM :)

kapilkd13 added 2 commits August 7, 2017 23:40

harcoded tmp folder prevent windows compatibilty of past module

53d0618

correcting TMP environment None error

37c8664

kapilkd13 mentioned this pull request Aug 8, 2017

document /tmp fix for Windows+Py3 common-workflow-language/schema_salad#126

Closed

3 tasks

mr-c suggested changes Aug 8, 2017

View reviewed changes

using tempfile.mkdtemp to create a temp directory for writing file onto

5eef178

mr-c approved these changes Aug 8, 2017

View reviewed changes

serge2016 approved these changes Oct 11, 2017

View reviewed changes

bakkerthehacker reviewed Apr 9, 2018

View reviewed changes

bakkerthehacker mentioned this pull request Apr 9, 2018

Remove unused tmp files from past translation #335

Merged

jmadler suggested changes Sep 17, 2018

View reviewed changes

jmadler closed this May 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

harcoded tmp folder prevent windows compatibilty of past module #296

harcoded tmp folder prevent windows compatibilty of past module #296

kapilkd13 commented Aug 7, 2017

mr-c Aug 8, 2017

kapilkd13 Aug 8, 2017

mr-c left a comment

serge2016 commented Oct 10, 2017

mr-c commented Oct 10, 2017

kapilkd13 commented Oct 10, 2017

serge2016 commented Oct 11, 2017

kapilkd13 commented Oct 11, 2017

serge2016 commented Oct 11, 2017 •

edited

serge2016 commented Nov 9, 2017

krischer commented Nov 9, 2017

serge2016 commented Nov 9, 2017

serge2016 commented Dec 7, 2017

d1skort commented Dec 27, 2017

theferrit32 commented Jan 11, 2018

d1skort commented Mar 26, 2018

ADKosm commented Mar 26, 2018

mr-c commented Mar 26, 2018

serge2016 commented Mar 26, 2018

mr-c commented Mar 26, 2018

kapilkd13 commented Apr 1, 2018

mr-c commented Apr 1, 2018 •

edited

kapilkd13 commented Apr 1, 2018

bakkerthehacker Apr 9, 2018

jmadler Sep 17, 2018

bakkerthehacker Sep 17, 2018

theferrit32 commented Apr 10, 2018

serge2016 commented Sep 12, 2018

kapilkd13 commented Sep 12, 2018

jmadler Sep 17, 2018

jmadler commented Sep 17, 2018 •

edited

theferrit32 commented Sep 17, 2018

jmadler commented May 6, 2019

harcoded tmp folder prevent windows compatibilty of past module #296

harcoded tmp folder prevent windows compatibilty of past module #296

Conversation

kapilkd13 commented Aug 7, 2017

mr-c Aug 8, 2017

Choose a reason for hiding this comment

kapilkd13 Aug 8, 2017

Choose a reason for hiding this comment

mr-c left a comment

Choose a reason for hiding this comment

serge2016 commented Oct 10, 2017

mr-c commented Oct 10, 2017

kapilkd13 commented Oct 10, 2017

serge2016 commented Oct 11, 2017

kapilkd13 commented Oct 11, 2017

serge2016 commented Oct 11, 2017 • edited

serge2016 commented Nov 9, 2017

krischer commented Nov 9, 2017

serge2016 commented Nov 9, 2017

serge2016 commented Dec 7, 2017

d1skort commented Dec 27, 2017

theferrit32 commented Jan 11, 2018

d1skort commented Mar 26, 2018

ADKosm commented Mar 26, 2018

mr-c commented Mar 26, 2018

serge2016 commented Mar 26, 2018

mr-c commented Mar 26, 2018

kapilkd13 commented Apr 1, 2018

mr-c commented Apr 1, 2018 • edited

kapilkd13 commented Apr 1, 2018

bakkerthehacker Apr 9, 2018

Choose a reason for hiding this comment

jmadler Sep 17, 2018

Choose a reason for hiding this comment

bakkerthehacker Sep 17, 2018

Choose a reason for hiding this comment

theferrit32 commented Apr 10, 2018

serge2016 commented Sep 12, 2018

kapilkd13 commented Sep 12, 2018

jmadler Sep 17, 2018

Choose a reason for hiding this comment

jmadler commented Sep 17, 2018 • edited

theferrit32 commented Sep 17, 2018

jmadler commented May 6, 2019

serge2016 commented Oct 11, 2017 •

edited

mr-c commented Apr 1, 2018 •

edited

jmadler commented Sep 17, 2018 •

edited