-
Notifications
You must be signed in to change notification settings - Fork 12
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
Updates the instruction about wheel builds and syncing #67
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kushaldas for these docs update, they are quite clear and concise.
In order to test this PR, I simply tried to go through all the steps: I had securedrop-client repo checked out on the latest version of master a4ef73955f493605352afb25aca72e7165d528eb. This repo checked out at 7ff5909 and I reverted 997014b to try building the securedrop-sdk-0.0.11.
I got to step 4, and ran into an issue while building the folders for the html at step 4. I've also added a couple of comments, see inline.
README.md
Outdated
|
||
### 1. Sync the wheels locally | ||
|
||
Sync all of the latest wheels `make syncwheels` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no make syncwheels
target in this repo. Do you mean make fetch-wheels
, or am I missing something?
|
||
|
||
``` | ||
PKG_DIR=/home/user/code/securedrop-client make build-wheels |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After building the wheels, I get the following message:
Successfully built alembic arrow certifi chardet idna mako markupsafe pathlib2 python-dateutil python-editor requests securedrop-sdk six sqlalchemy urllib3
Copying securedrop-sdk-0.0.11.tar.gz to cache ./localwheels
Copying securedrop_sdk-0.0.11-py3-none-any.whl to cache ./localwheels
./scripts/sync-sha256sums
Now you must sign the generated sha256sums.txt file.File '../sha256sums.txt.asc' exists. Overwrite? (y/N) y
./scripts/createdownloadurls.py > wheelsurls.txt
Done! Now please follow the instructions in
https://github.com/freedomofpress/securedrop-debian-packaging-guide/issues/6
to push these changes to the FPF PyPI index
Perhaps we should update this message to refer to this README instead, since it should become the canonical docs for maintainers to build wheels?
This above command will let you know about any new wheels+sources. It will build/download sources from PyPI (by verifying it against the sha256sums from the `Pipfile.lock` of the project). | ||
|
||
``` | ||
python3 setup.py sdist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to navigate into the project's repo for this?
securedrop-debian-packaging$ python3 setup.py sdist
python3: can't open file 'setup.py': [Errno 2] No such file or directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I will clear that part in this PR.
If there is any new package (source/wheel), then we will have to update our index. | ||
|
||
``` | ||
./scripts/createdirs.py ~/code/securedrop-client/requirements.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getting an error here, it appears to have an issue parsing the requirements file for securedrop-client@a4ef73955f493605352afb25aca72e7165d528eb . Am I missing something?
securedrop-debian-packaging$ ./scripts/createdirs.py ~/src/securedrop-client/requirements.txt
Traceback (most recent call last):
File "./scripts/createdirs.py", line 25, in <module>
assert len(words) == 2
AssertionError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this will also need update. This is only when we add a complete new module as dependency.
README.md
Outdated
``` | ||
|
||
|
||
### 3. Sync the localwheels directory back to the s3 bucket. (if only any update) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
propose changing if only any update
to if any wheels were updated
for clarity
README.md
Outdated
|
||
### 3. Sync the localwheels directory back to the s3 bucket. (if only any update) | ||
|
||
This has to be manual step for security reason. In future all of these wheel building steps should be done by a different system, not at the devloper's laptop. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reason-> reasons
not at the devloper laptop -> not with the developer's laptop
README.md
Outdated
``` | ||
Then update the corresponding packages's `index.html`. | ||
|
||
If new package, then update the main index. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If new package -> If there is a new package
After these steps, please rerun the command again. | ||
``` | ||
|
||
So, the next step is to build the wheels. To do this step, you will need the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case I propose saying something like "As of now, this can only be done by maintainers, as only their gpg keys are trusted for the signature of the wheel hashes" or something of the sort
I will make the changes suggested by @emkll and push a new version in the morning. |
@emkll I will get back to this on Friday. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kushaldas for the fixes, these docs look good to me.
Updates
README.md
with instructions.Fixes #55