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.

Sign up for GitHub

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

Jump to bottom

add -r support to setup.py #56335

Closed
wants to merge 17 commits into from
Closed

add -r support to setup.py #56335

wants to merge 17 commits into from

Conversation

cmcmarrow
Copy link
Contributor

@cmcmarrow cmcmarrow commented Mar 9, 2020 •
edited
Loading

What does this PR do?

lets salt-bin build on windows

New Behavior

"-r " is now ran in setup.py

Tests written?

no

Commits signed with GPG?

Yes

@cmcmarrow cmcmarrow requested a review from a team as a code owner March 9, 2020 21:23
@ghost ghost requested a review from DmitryKuzmenko March 9, 2020 21:23
Akm0d
Akm0d previously approved these changes Mar 9, 2020
View reviewed changes
xeacott
xeacott previously approved these changes Mar 9, 2020
View reviewed changes
dmurphy18
dmurphy18 reviewed Mar 9, 2020
View reviewed changes
Copy link
Contributor

@dmurphy18 dmurphy18 left a comment

Choose a reason for hiding this comment

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

Worried that fixing this issue for 3000.1 will change current mode of operation (even if it is wrong) and now building will have a slew of new requirements appear, similar to what happened with the change from distutils to setuptools in 3000. Perhaps this would be better postponed till Sodium. But open to persuasion on the matter

@codecov
Copy link

codecov bot commented Mar 9, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@602ff3b). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #56335   +/-   ##
=========================================
  Coverage          ?   23.02%           
=========================================
  Files             ?      286           
  Lines             ?    64304           
  Branches          ?    14711           
=========================================
  Hits              ?    14802           
  Misses            ?    47607           
  Partials          ?     1895
Flag Coverage Δ
#centos7 23.02% <ø> (?)
#proxy 23.02% <ø> (?)
#py3 23.02% <ø> (?)
#runtests 23.02% <ø> (?)
#zeromq 23.02% <ø> (?)

s0undt3ch
s0undt3ch reviewed Mar 9, 2020
View reviewed changes
Copy link
Collaborator

@s0undt3ch s0undt3ch left a comment

Choose a reason for hiding this comment

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

I would advise against merging this for 3000.1

There's an open PR which fixes the requirements for 3000.1 in a less intrusive way.

#56099

For Sodium, as long as the packaging is not broken, I'm ok with this change.

@cmcmarrow cmcmarrow requested a review from s0undt3ch March 9, 2020 22:51
@cmcmarrow
Copy link
Contributor Author

cmcmarrow commented Mar 9, 2020 •
edited
Loading

Windows and Apple are the only two that have -r from what I can see. The normal Windows installer catches all missing modules that this catches. The only place I can see go wrong is Apple and that's just because I don't know how the process.

twangboy
twangboy requested changes Mar 9, 2020
View reviewed changes
setup.py Outdated Show resolved Hide resolved
@cmcmarrow cmcmarrow requested a review from twangboy March 18, 2020 19:52
DmitryKuzmenko
DmitryKuzmenko previously approved these changes Apr 6, 2020
View reviewed changes
@s0undt3ch
Copy link
Collaborator

This is superseded by #56904

@s0undt3ch s0undt3ch mentioned this pull request Apr 30, 2020
Merged
@DmitryKuzmenko
Copy link
Contributor

@s0undt3ch maybe close it then?

@s0undt3ch
Copy link
Collaborator

It will get closed when the linked PR is merged.

@cmcmarrow cmcmarrow dismissed stale reviews from DmitryKuzmenko, xeacott, and Akm0d via c2a819c May 15, 2020 19:33
@DmitryKuzmenko
Copy link
Contributor

Hm... So this one or #56904?

@DmitryKuzmenko
Copy link
Contributor

@s0undt3ch ^^^

@DmitryKuzmenko
Copy link
Contributor

Just checked this part in #56904 it also handles an inline hash-comments.
I'd better like #56904

@s0undt3ch
Copy link
Collaborator

None for Sodium.
If the issue is salt-bin, we'll address it when building(again for Sodium).

@cmcmarrow
Copy link
Contributor Author

@s0undt3ch the PR is open due to salt-bin.

@dwoz dwoz added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmurphy18 dmurphy18 dmurphy18 approved these changes

@Akm0d Akm0d Akm0d approved these changes

@DmitryKuzmenko DmitryKuzmenko DmitryKuzmenko approved these changes

@s0undt3ch s0undt3ch Awaiting requested review from s0undt3ch

@twangboy twangboy Awaiting requested review from twangboy

@xeacott xeacott Awaiting requested review from xeacott

Assignees

@cmcmarrow cmcmarrow

Labels
Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@cmcmarrow @s0undt3ch @DmitryKuzmenko @Akm0d @twangboy @dmurphy18 @xeacott @dwoz