Skip to content
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

Add a note about temporary directories to --build help text #4944

Merged
merged 11 commits into from
Jan 1, 2018

Conversation

1byxero
Copy link
Contributor

@1byxero 1byxero commented Dec 26, 2017

Added documentation on the usage of command line argument --build
Closes #4262

@pradyunsg pradyunsg self-assigned this Dec 26, 2017
Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @1byxero! Thanks for this PR!

Just want a few minor grammar related changes. =)

(hopefully I don't come across as that annoying fellow who corrects your grammar) :P

@@ -434,7 +434,10 @@ def only_binary():
'-b', '--build', '--build-dir', '--build-directory',
dest='build_dir',
metavar='dir',
help='Directory to unpack packages into and build in.'
help='Directory to unpack packages into and build in. '
'Initial build would still take place in direcotry set by TMPDIR '
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • typo: directory

help='Directory to unpack packages into and build in.'
help='Directory to unpack packages into and build in. '
'Initial build would still take place in direcotry set by TMPDIR '
'environment variable, (TEMP on windows) To change this behavior,'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • an extra comma here after variable
  • To is capitalised in the middle of the sentence
  • windows should be Windows
  • there is no space after the last comma

help='Directory to unpack packages into and build in. '
'Initial build would still take place in direcotry set by TMPDIR '
'environment variable, (TEMP on windows) To change this behavior,'
'set appropriate directory by setting TMPDIR environment variable'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Missing a trailing period

@pradyunsg
Copy link
Member

Actually, on second thought, could you rephrase this to read as:

Directory to unpack packages into and build in. Note that an initial build still takes place in a temporary directory. The location of temporary directories can be controlled by setting the TMPDIR environment variable (TEMP on Windows) appropriately.

Thanks! :)

@pradyunsg
Copy link
Member

@1byxero another thing you might want to do -- if you add the words "Closes #4262" to this PR description (the first comment by you), the relevant issue would be auto closed when this PR is merged. :)

@1byxero
Copy link
Contributor Author

1byxero commented Jan 1, 2018

Sure. Will do all the changes you mentioned!

@1byxero 1byxero changed the title Documentation for setting up of a tmpdir, addresses issue #4262 Documentation for setting up of a tmpdir, addresses and Closes #4262 issue Jan 1, 2018
Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, will merge if the CI approves.

=)

@pradyunsg
Copy link
Member

Linters don't approve. One of the lines is too long and the indentation should be such that the quote of the continuation matches that of the line before it. Could you make these changes as well @1byxero? =)

'Note that an initial build still takes place in a temporary directory. '
'The location of temporary directories can be controlled by setting '
help='Directory to unpack packages into and build in. Note that '
'an initial build still takes place in a temporary directory. The '
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a space in this indentation.

Copy link
Contributor Author

@1byxero 1byxero Jan 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes i have checked the CI logs and have updated the help text accordingly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ill monitor those things, and will fix the issues if any, will notify you with a mention when things will be looking fine

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! No hurries. :)

To run the linters locally, run tox -e lint-py3

@1byxero
Copy link
Contributor Author

1byxero commented Jan 1, 2018

Yes updating those things to match the pep8 standards

@pradyunsg pradyunsg changed the title Documentation for setting up of a tmpdir, addresses and Closes #4262 issue Add a note about temporary directories to --build help text Jan 1, 2018
@1byxero
Copy link
Contributor Author

1byxero commented Jan 1, 2018

@pradyunsg hey So this CI looks good to me except packaging which failed due to

oci runtime error: exec failed: container_linux.go:265: starting container process caused "could not create session key: disk quota exceeded"

I ran the tox -e packaging locally and it succeded
So is there a way to restart a specific test so that we can retry that?

@pradyunsg
Copy link
Member

I've restarted it.

@1byxero
Copy link
Contributor Author

1byxero commented Jan 1, 2018

Looks like all checks have passed @pradyunsg
Pretty excited about getting my PR approved for pip.
One small question, when will this piece of code will be moved to the production version which will be used by the masses?

@pradyunsg
Copy link
Member

Pretty excited about getting my PR approved for pip.

Congratulations! 🎉

when will this piece of code will be moved to the production version which will be used by the masses?

It'll be a part of pip 10, which is the next major release of pip. We don't really follow a release schedule but we're planning on doing one soon, once some work related to PEP 518 and all happens.

@pradyunsg
Copy link
Member

@pypa/pip-committers Could one of you also take a look at this?

I'll be happy to merge once there's another approval on this. :)

@1byxero
Copy link
Contributor Author

1byxero commented Jan 1, 2018

@pradyunsg Amazing!
Thanks.

@pfmoore
Copy link
Member

pfmoore commented Jan 1, 2018

Looks good to me - congratulations @1byxero and thanks for the contribution!

@pradyunsg pradyunsg merged commit f038fe4 into pypa:master Jan 1, 2018
@lock
Copy link

lock bot commented Jun 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 2, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

document how to set tmpdir
3 participants