Package version data #122

kostiakoval · 2016-02-03T19:11:10Z

Work in Progress
This is initial commit for SR-473

We need to generate version data for every package on build.
Now it creates a new folder versionData inside .build dir and creates a swift file for every package.

Could you please provide any feedback for the initial implementation.

Should these files be inside .build dir or somewhere else?
Should we generate a new .swift file for every Package or place all data in 1 file?
Any other suggestion?

ToDo:

Add unit test
Add better error handling
Merge PR add posix time formatting #225
add additional information to Root package (local changes)

mxcl · 2016-02-03T21:55:22Z

Ideally these would be targets integrated into the build yaml so they are not generated every time. However not doing it this way at first is acceptable since we don't have a good way to specify the version dependency yet.

aciidgh · 2016-02-04T11:06:46Z

Sources/swift-build/versionData.swift

+
+    try fputs("public let url: String = \"\(package.url)\" \n", file)
+    let version = package.version
+    try fputs("public let version: (Int, Int, Int, [String], String?) = " +


I think this would be better if tuple had named values?

@aciidb0mb3r something like:

public let version: (major: Int, minor: Int, patch: Int, etc: [String], metadata: String?)

I think using type inference would be even better:
What do you think @aciidb0mb3r @mxcl ?

public let version: (major: Int, minor: Int, patch: Int, etc: [String], metadata: String?) = (1, 0, 1, [], "") //vs public let version = (1, 0, 1, [], "")

😞 Unfortunately it's not possible, not enough info

imo, it would be fine if it was just public let version = (1, 0, 1) as this is guessable but I have no way of knowing what is last two fields in public let version = (1, 0, 1, [], "") without seeing doc (as they'll be empty for most packages)

It's Version's properties.

prereleaseIdentifiers: [String]

buildMetadataIdentifier: String?
They describe a version tag of a package. So we need them.

mxcl · 2016-03-08T03:20:52Z

Let’s resurrect this potentially fabulous addition.

kostiakoval · 2016-03-09T08:42:35Z

I'm sorry for being lazy here. I'm going to update it soon

mxcl · 2016-03-10T00:03:35Z

“Lazy” is certainly not the right word. But we owe it to everyone to prod each other and keep momentum up :)

kostiakoval · 2016-03-17T22:25:11Z

Hi all. I'm back :)

I'm thinking of adding version: Version? property to a Packagetype. It's optional because local package could be missing a git version.
What do you think @mxcl?

jessesquires · 2016-03-17T23:58:43Z

@kostiakoval an optional seems like a bad idea to me.

@mxcl isn't this something that SPM intends to enforce?

i'm not sure how effect a package manager could be without version numbers 😄

kostiakoval · 2016-03-18T08:05:17Z

Hi @jessesquires I'm not talking about a Version you specify in your Manifest file when you describe a package, that must be specified and it's not optional.

The Version I described, is a version we get when we fetch a package.

Local packages .Package(url: "../Foo", versions: Version(1,0,0)..<Version(2,0,0))
It could have missing version tag, and that should be fine when you are developing a package (not published yet).
This PR, represents a metadata (url and Version) for every package swiftpm is fetching and building.

Does it make any sense, any suggestion how make it better?

jessesquires · 2016-03-18T16:12:50Z

Ah! 😊 I see. In that case, we probably do want an optional.

Maybe we could rename to versionTag or something similar to avoid confusion?

mxcl · 2016-03-18T16:48:08Z

No optional, I think in the case of the root package where we can't get a version just go with 0,0,0. It would suck to always check for the optional here and need to code an error path when it is in fact a programmer error,

The other option is Version!

mxcl · 2016-03-18T16:49:00Z

Unless this is only being exposed to SwiftPM, which seems likely, so Version? is ok in that case.

kostiakoval · 2016-03-20T16:31:11Z

Done. Review it please.

On build generate swift file for every package with Version data.
Each file contains:

public let url: String
public let version: (Int, Int, Int, [String], String?)?

kostiakoval · 2016-03-22T13:56:18Z

@mxcl Can you please run CI and merge it?

aciidgh · 2016-03-23T18:44:49Z

Sources/Build/PackageVersion.swift

+import func libc.fclose
+
+public func generateVersionData(rootDir: String, packages: [Package]) throws {
+    let dirPath = rootDir + "/.build/versionData/"


prefer Path.join

kostiakoval · 2016-03-23T19:10:42Z

@aciidb0mb3r Thanks for feedback. Your comments are fixed now 👍

mxcl · 2016-03-25T17:39:28Z

I've changed my view here.

If root package version data cannot be obtained, do not generate the version swift.

If it can be obtained but commits or work has been done since a version, then add a build identifier with the date.

kostiakoval · 2016-03-28T08:18:19Z

@mxcl Can we review and merge this PR? Some parts of it can be reused when implementing PackageVersions.json file

ddunbar · 2016-04-11T18:09:41Z

Maybe I don't understand, how would other code use them if they aren't optional?

mxcl · 2016-04-11T18:33:48Z

I think you understand: other code can't use them if they don't exist.

Thus they won't exist:

For new packages not yet ever published
Projects that never intend to have versions

I think it is reasonable for version information to not exist when it literally doesn't exist.

mxcl · 2016-04-11T19:20:34Z

K, talked with Daniel and we think:

version should not be optional, if there is no version, don't generate it. We think this is reasonable so that: people don't have to add error handling for when the version doesn't exist considering the times it doesn't exist are very edge case
We agree sha should remain optional and we like how it works
We think the modified string should be changed to a single Bool. Our thinking here is that we don't think the build-time has much utility in the context of this PR and we really don't want people writing code that does things based on this date value.
Let's label all of the tuple values in version.
Let’s add a versionString property that returns a string since mostly this is a feature for logging versions

Additional longer term considerations:

We think that this should only be generated for the root-package, the rationale here is that we don't want packages writing code like if version.major > 2 as this should be a compile time language feature and Swift core are already on board with this idea.
Thus we think we should generate a single module called PackageVersions (depending on the version-pinning proposal’s end name)
And thus we should probably define a struct for each package in that file

I'll let Daniel correct me if he has any corrections. Otherwise, thoughts?

ddunbar · 2016-04-11T22:02:59Z

I thought we agreed for assigning let version = (0,0,0,...) if missing -- I think this is better so that you can test the code which would use it before you have tagged. Otherwise you have to tag, build, test, retag.

Otherwise yup that matches my thoughts.

One thing to note about the implementation is you will need to treat PackageVersions as a dependency of every target in the top-level project (so they can import it, potentially).

mxcl · 2016-04-11T22:29:53Z

I thought we agreed for assigning let version = (0,0,0,...) if missing -- I think this is better so that you can test the code which would use it before you have tagged. Otherwise you have to tag, build, test, retag.

Oh yes, sorry. You are right. I agree.

mxcl · 2016-04-11T22:30:13Z

One thing to note about the implementation is you will need to treat PackageVersions as a dependency of every target in the top-level project (so they can import it, potentially).

Indeed. Though @kostiakoval it is cool if we do this part of the implementation in a separate PR.

kostiakoval · 2016-04-12T06:28:09Z

We think that this should only be generated for the root-package,

Can you please clarify what you mean by root-package

It's a package with Package.swift file and it's always only 1. For swiftpm it's a package with "SwiftPM" name.
Did I understood it correctly?

We should generate 1 swift file for a root-package. That file will contain version info about root-package and it's dependencies.

mxcl · 2016-04-12T06:44:34Z

When we say root package we mean the one swift build is executing against.

kostiakoval · 2016-04-13T19:57:32Z

I've addressed 5 mentioned points:

version should not be optional,
keep sha optional
modified as a Bool
add label to all of the tuple values in version.
Let’s add a versionString

Few questions:

should we generate the same version data format for root-package and its dependencies?
now dependency package don't include sha and modified
I think they should have same format.
the url show the local path for a root-package. Should we use git origin if possible
if there is no git repo for a root-package, should we still generate PackageVersions.swift file ?
It is possible since we have local path as url and default version value

mxcl · 2016-04-13T21:06:41Z

should we generate the same version data format for root-package and its dependencies? now dependency package don't include sha and modified I think they should have same format.

My perspective is: yes.

the url show the local path for a root-package. Should we use git origin if possible

Yes, use the origin if possible.

if there is no git repo for a root-package, should we still generate PackageVersions.swift file ? It is possible since we have local path as url and default version value

I think we can be consistent and if there is no git data:

Version(0,0,0)
local path instead of origin

mxcl · 2016-04-14T16:29:37Z

I'm happy to merge this if you are.

mxcl · 2016-04-14T16:29:44Z

@swift-ci Please test

kostiakoval · 2016-04-14T17:00:30Z

I'm implementing these changes. Will be done in 30 min

kostiakoval · 2016-04-14T18:23:53Z

I don't feel this is optimal solution. I've tried to fix it but found more issue. We can merge it and improve it later if you like

mxcl · 2016-04-14T18:56:08Z

We aim for more incremental development. It doesn't need to be perfect to be landed.

kostiakoval · 2016-04-14T19:00:20Z

Agree. Let's wait for tests first

kostiakoval · 2016-04-14T20:15:01Z

@mxcl Test are passing. Can we merge it. It would be easier to do smaller improvements than

Minor fixes

aciidgh reviewed Feb 4, 2016
View reviewed changes

kostiakoval force-pushed the package-versionData branch from 7894638 to 44a7f4c Compare March 17, 2016 22:18

kostiakoval changed the title ~~[WIP] Package version data~~ Package version data Mar 20, 2016

kostiakoval force-pushed the package-versionData branch 3 times, most recently from aa193c8 to 9549f1d Compare March 22, 2016 20:22

kostiakoval mentioned this pull request Mar 23, 2016

Add test for Test module. #143

Closed

aciidgh reviewed Mar 23, 2016
View reviewed changes

kostiakoval force-pushed the package-versionData branch from 9549f1d to e218e21 Compare March 23, 2016 19:09

kostiakoval force-pushed the package-versionData branch from e218e21 to 466f603 Compare March 25, 2016 10:08

kostiakoval mentioned this pull request Mar 27, 2016

add posix time formatting #225

Merged

kostiakoval force-pushed the package-versionData branch from 7773b49 to 4e4bf44 Compare April 7, 2016 19:44

kostiakoval force-pushed the package-versionData branch 2 times, most recently from d93e2d9 to ac83b46 Compare April 13, 2016 18:43

kostiakoval added 3 commits April 13, 2016 20:48

generate versionData swift file for package

8ae4a22

generate version data for root package

77a75d0

save commit SHA and local changes date for root package

4d6b92c

kostiakoval force-pushed the package-versionData branch from ac83b46 to b2cb148 Compare April 13, 2016 18:49

change packageVersion data format

db36292

kostiakoval force-pushed the package-versionData branch from b2cb148 to db36292 Compare April 13, 2016 19:57

mxcl merged commit 5389d99 into swiftlang:master Apr 14, 2016

kostiakoval deleted the package-versionData branch April 15, 2016 05:54

rfdickerson mentioned this pull request Apr 18, 2016

Added ServerInfo and Server response header Kitura/Kitura#352

Closed

3 tasks

aciidgh pushed a commit to aciidgh/swift-package-manager that referenced this pull request Jan 11, 2019

Merge pull request swiftlang#122 from neonichu/minor-fixes

a1db72a

Minor fixes

swift-ci mentioned this pull request Feb 3, 2016

[SR-473] Add version constants to each package #5327

Open

Package version data #122

Package version data #122

Uh oh!

Conversation

kostiakoval commented Feb 3, 2016

Uh oh!

mxcl commented Feb 3, 2016

Uh oh!

aciidgh Feb 4, 2016

Choose a reason for hiding this comment

Uh oh!

kostiakoval Feb 4, 2016

Choose a reason for hiding this comment

Uh oh!

aciidgh Feb 4, 2016

Choose a reason for hiding this comment

Uh oh!

kostiakoval Feb 4, 2016

Choose a reason for hiding this comment

Uh oh!

aciidgh Feb 4, 2016

Choose a reason for hiding this comment

Uh oh!

kostiakoval Feb 4, 2016

Choose a reason for hiding this comment

Uh oh!

mxcl commented Mar 8, 2016

Uh oh!

kostiakoval commented Mar 9, 2016

Uh oh!

mxcl commented Mar 10, 2016

Uh oh!

kostiakoval commented Mar 17, 2016

Uh oh!

jessesquires commented Mar 17, 2016

Uh oh!

kostiakoval commented Mar 18, 2016

Uh oh!

jessesquires commented Mar 18, 2016

Uh oh!

mxcl commented Mar 18, 2016

Uh oh!

mxcl commented Mar 18, 2016

Uh oh!

kostiakoval commented Mar 20, 2016

Uh oh!

kostiakoval commented Mar 22, 2016

Uh oh!

aciidgh Mar 23, 2016

Choose a reason for hiding this comment

Uh oh!

kostiakoval commented Mar 23, 2016

Uh oh!

mxcl commented Mar 25, 2016

Uh oh!

kostiakoval commented Mar 28, 2016

Uh oh!

ddunbar commented Apr 11, 2016

Uh oh!

mxcl commented Apr 11, 2016

Uh oh!

mxcl commented Apr 11, 2016

Uh oh!

ddunbar commented Apr 11, 2016

Uh oh!

mxcl commented Apr 11, 2016

Uh oh!

mxcl commented Apr 11, 2016

Uh oh!

kostiakoval commented Apr 12, 2016

Uh oh!

mxcl commented Apr 12, 2016

Uh oh!

kostiakoval commented Apr 13, 2016

Uh oh!

mxcl commented Apr 13, 2016

Uh oh!

mxcl commented Apr 14, 2016

Uh oh!

mxcl commented Apr 14, 2016