Skip to content

Added support for macOS#147

Merged
chinmayshah99 merged 21 commits intodevfrom
madhavajay/macos
May 29, 2020
Merged

Added support for macOS#147
chinmayshah99 merged 21 commits intodevfrom
madhavajay/macos

Conversation

@madhavajay
Copy link
Copy Markdown
Contributor

@madhavajay madhavajay commented May 18, 2020

  • Removed third_party folder
  • Updated Bazel WORKSPACE
  • Fixed paths in example/carrots.py
  • Added Pipfile for virtualenv
  • Added build_macos.sh script

Fixes #143

Type of change

Please mark options that are relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Yes the carrots demo runs.

- Removed third_party folder
- Updated Bazel WORKSPACE
- Fixed paths in example/carrots.py
- Added Pipfile for virtualenv
- Added build_macos.sh script
@chinmayshah99 chinmayshah99 changed the title Added support for macOS WIP: Added support for macOS May 19, 2020
@chinmayshah99 chinmayshah99 changed the title WIP: Added support for macOS Added support for macOS May 23, 2020
@chinmayshah99 chinmayshah99 requested a review from replomancer May 23, 2020 21:12
@chinmayshah99 chinmayshah99 mentioned this pull request May 25, 2020
8 tasks
iamtrask
iamtrask previously approved these changes May 29, 2020
Copy link
Copy Markdown
Member

@iamtrask iamtrask left a comment

Choose a reason for hiding this comment

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

Looks great to me! But I’d wait until another review before merging.

- Fixed pydp.so cleanup before new build
@madhavajay
Copy link
Copy Markdown
Contributor Author

Hrm... looks like i have broken things.
I tried to add an update but because i'm no longer authorized to push here I need to push to my fork, which auto updates this PR somehow... So my last commit has leapt forward to today which is weird.

I actually was trying to get this Commit in.
madhavajay@6d2b313

I made the mistake of thinking maybe I should try the GitHub Desktop app and I think that just made it worse with some weird merge strategy auto stuff... I should have manually force pushed this branch up onto my fork first. Now I am hesitant to do anything in case I make it worse. If someone wants to make the change above I linked to, I believe it fixes an error in the xargs command on linux CI.
https://github.com/OpenMined/PyDP/runs/718826904?check_suite_focus=true

INFO: Build completed successfully, 637 total actions
INFO: Build completed successfully, 637 total actions
rm: missing operand
Try 'rm --help' for more information.

But this needs to be confirmed, although its not really needed on CI since its a clean environment which would explain why this didn't get noticed. We could possibly put something like:

set -o pipefail

That way any error will cause the test to fail so we don't miss silent errors.

@madhavajay
Copy link
Copy Markdown
Contributor Author

Other than that update, I think the PR is good! 👍

@madhavajay
Copy link
Copy Markdown
Contributor Author

Also how do we feel about Squash and Force Push on branches like these before Merging the PR?

* added publishing for pypi

* bazel is already installed lol

* twine needs to be run via pipenv

* Mac workflow test (#6)


* Update pypipublish_osx.yml

* cleaning up files after testing
@chinmayshah99
Copy link
Copy Markdown
Member

chinmayshah99 commented May 29, 2020

I actually was trying to get this Commit in.
madhavajay/PyDP@6d2b313

Manually adding this.

@chinmayshah99
Copy link
Copy Markdown
Member

@madhavajay
When I add make changes in my local machine as mentioned in madhavajay/PyDP@6d2b313:

INFO: Build completed successfully, 1 total action
xargs: WARNING: a NUL character occurred in the input.  It cannot be passed through in the argument list.  Did you mean to use the --null option?

@chinmayshah99 chinmayshah99 merged commit 37385af into dev May 29, 2020
@delete-merged-branch delete-merged-branch Bot deleted the madhavajay/macos branch May 29, 2020 15:16
@chinmayshah99
Copy link
Copy Markdown
Member

chinmayshah99 commented May 29, 2020

Also how do we feel about Squash and Force Push on branches like these before Merging the PR?

If the code does one functionality then yes.
In this case, this repo had a couple of things that were fixed among like 20+ files. So it made more sense to merge commit.
In the pypi #136 , we did squash commit.

@madhavajay
Copy link
Copy Markdown
Contributor Author

Regarding the xargs error, its always a challenge to get bash scripts to work perfectly on linux and mac, but I should have actually tested it on a local docker container first. Sorry for that, this works and has been tested on both platforms:

find ./ -name pydp.so -print0 | xargs -0 -I {} rm {}

For the commits I just mean there are a lot of "fixing errors" commits in this PR which could be squashed into a single commit. Its just a style preference and doesn't really matter but makes reviewing the whole PR in the longer history of the repo a little easier.

@chinmayshah99
Copy link
Copy Markdown
Member

This looks great. Can you make a PR if you don't mind?

dvadym pushed a commit to dvadym/PyDP that referenced this pull request Jul 3, 2022
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.

Add support for OSX

3 participants