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

Include _version.py if building wheel #56378

Merged
merged 6 commits into from
Mar 17, 2020
Merged

Conversation

Ch3LL
Copy link
Contributor

@Ch3LL Ch3LL commented Mar 14, 2020

What does this PR do?

Ensures the _version.py file is included when building the bdist_wheel package. While writing tests and running tests for this found a couple more issues that this PR cleans up:

  1. name is not included in repr for the new version scheme, and it includes bugfix even if we are using new version scheme:

before:

>>> salt.version.SaltStackVersion(3000, 1, None, None, '', 0, 0, None)
<SaltStackVersion major=3000 minor=1 bugfix=None>

after:

>>> salt.version.SaltStackVersion(3000, 1, None, None, '', 0, 0, None)
<SaltStackVersion name='Neon' major=3000 minor=1>
  1. When adding the version to _version.py it uses full_info and then instantiates another instance of SaltStackVersion in that file using full_info. The problem with this is full_info will not return the bugfix, mbugfix or minor if it doesn't exist with the new versioning scheme. This leads to the pre_type, pre_num, noc, or sha then becoming the bufix version for example. This change adds a new property method full_info_all_versions which returns the old versioning scheme so when we instantiate a new object we return the correct major,minor,etc.

To display this:

before:

In [7]: first = salt.version.SaltStackVersion(3000, 1, None, None, '', 0, 0, None)        

In [8]: first.pre_type                       
Out[8]: ''

In [9]: second = salt.version.SaltStackVersion(*first.full_info)                          

In [10]: second.pre_type                     
Out[10]: 0

as you can see above, full_info changed what we were expecting because full_info should only return the new versioning scheme. But this causes problems when we use this full_info to instantiate another SaltStackVersion. So i created another propertymethod for this use case:

In [11]: second = salt.version.SaltStackVersion(*first.full_info_all_versions)            

In [12]: second.pre_type                     
Out[12]: ''

fyi @s0undt3ch thats why you were seeing this: #56358
this should also fix it so we dont get '' as the minor type. we are now using full_info_all_versions during setup.py when creating _version.py

What issues does this PR fix or reference?

#56377

Previous Behavior

[root@ch3ll ~]# salt --version
salt 3000

New Behavior

[root@ch3ll ~]# salt --version
salt 3000.1

Tests written?

Yes

Commits signed with GPG?

Yes

@Ch3LL Ch3LL requested a review from a team as a code owner March 14, 2020 05:05
@ghost ghost requested a review from dwoz March 14, 2020 05:05
@Ch3LL Ch3LL requested a review from s0undt3ch March 14, 2020 05:06
@Ch3LL Ch3LL added the v3000.1 vulnerable version label Mar 14, 2020
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.

Let use the existing helper instead.
Since you also patch the environ, the existing helper functionality could be extended to support that too.

In order to get .1 out faster I'm cool with switching my review but we should do the right thing then after the .1 release.

@@ -0,0 +1,77 @@
# -*- coding: utf-8 -*-
Copy link
Collaborator

Choose a reason for hiding this comment

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

We already have this helper.

except AttributeError:
# We're running off of the system python
kwargs = {}
self.run_function('virtualenv.create', [path], **kwargs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

run_function requires a master and minion running, last time I checked.
The existing virtualenv helper does not because it uses the sminion approach.

@s0undt3ch
Copy link
Collaborator

Also, great job finding out the real reason behind the empty string version minor issue.

setup.py Outdated
@@ -1224,6 +1229,9 @@ def parse_command_line(self):
if not self.ssh_packaging and PACKAGED_FOR_SALT_SSH:
self.ssh_packaging = 1

if 'bdist_wheel' in sys.argv:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just checking sys.argv also looks fragile but not to the point of holding up the release.
Let's merge when ready and then ping me and we'll talk on a possible better solution.

Again, this should work perfectly fine for the time being.

@dwoz dwoz merged commit e72a8d2 into saltstack:master Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v3000.1 vulnerable version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants