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

More Robust AVC Version Handling #1299

Merged
merged 7 commits into from
Aug 11, 2015
Merged

More Robust AVC Version Handling #1299

merged 7 commits into from
Aug 11, 2015

Conversation

dbent
Copy link
Member

@dbent dbent commented Jul 16, 2015

This PR makes the handling of version information found in internal .version files more robust. In practical experience the internal .version files aren't remarkably accurate, so we make the two following changes:

  • If the metadata already contains a version field, such as from KerbalStuff or a GitHub tag, we do not override it with the value found in the .version file.
  • KSP version information is calculated by "merging" the values that are already in the metadata and what we find in the .version file. A common example would be if some mod on KerbalStuff indicates that it's compatible with a version of KSP later than what it specifies in its .version file. In that case we calculate a minimum version from the .version file and a maximum version from the KerbalStuff metadata.
  • Add support for "build" component in AVC version, only added if present.

The CorrectlyCalculatesKspVersionInfo test contains a lot of cases. Please add any additional cases you think requires testing.

@pjf
Copy link
Member

pjf commented Jul 16, 2015

You are the best.

@JennDahm
Copy link

Would it be possible to indicate that we'd like for the AVC version to overwrite the GitHub version? I don't think the version tags I've been using in my project on GitHub aren't quite CKAN friendly (ex: "v0.1.1-alpha", rather than "0.1.1"), and it'd be nice if I could correct this via the AVC file I'm creating rather than recreating all the release tags.

@mheguy
Copy link
Contributor

mheguy commented Jul 19, 2015

@JonDahm For what it's worth we see quite a bit of v coming and going from one version to the next but thankfully have tools to work around that.

@JennDahm
Copy link

Just to be clear, you're saying that versions formatted with a v in front of the number work just as well as those without? If so, that's great and I don't have to worry about the version numbers comparing correctly.

@mheguy
Copy link
Contributor

mheguy commented Jul 19, 2015

It's not automatic, we have to add a flag and that flag automatically adds v to every version (so they're uniform). Because it's manual it means someone has to 'catch' that the v is inconsistent. If you have mods that you know use v intermittently just let us know and we can correct it.

@JennDahm
Copy link

Ah, okay. But so long as I use the v consistently, there shouldn't be a problem, correct?

@mheguy
Copy link
Contributor

mheguy commented Jul 19, 2015

Correct.

@dbent
Copy link
Member Author

dbent commented Jul 19, 2015

And if you really want to strip the v or other misc parts, #1321 will be an option.

@JennDahm
Copy link

Got it. Thanks a bunch!

mheguy added a commit to mheguy/CKAN-meta that referenced this pull request Jul 19, 2015
kspMaxes.Add(avcKspMax);

var kspMin = kspMins.Any() ? kspMins.Min() : null;
var kspMax = kspMaxes.Any() ? kspMaxes.Max() : null;
Copy link
Member

Choose a reason for hiding this comment

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

This is really clever, thank you! :)

@pjf
Copy link
Member

pjf commented Aug 11, 2015

@dbent : I'm so sorry this has been hanging so long, I'm still playing catch-up after being overseas, but this looks fantastic. Merging with great thanks!

pjf added a commit that referenced this pull request Aug 11, 2015
@pjf pjf merged commit 17b29f0 into master Aug 11, 2015
@pjf pjf removed the Pull request label Aug 11, 2015
pjf added a commit that referenced this pull request Aug 11, 2015
@dbent dbent deleted the topic/avc_no_override_version branch August 11, 2015 12:05
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.

None yet

4 participants