Adds `haxelib libpath` - Closes #407 #408

mastef · 2018-02-08T08:47:50Z

Adds haxelib libpath - Closes #407

EricBishton · 2018-02-08T08:57:57Z

I think you need to bump the version, too. So we can programatically know whether the command is available on the user's system.

EricBishton · 2018-02-08T08:59:40Z

src/haxelib/client/Main.hx

@@ -1344,6 +1345,25 @@ class Main {
 		}
 	}

+	function pathonly() {


OK, this is cheating. 😃 In the interest of not duplicating bugs, do you think that we can combine the contents of the two commands (path and pathonly) into one shared routine?

OK @EricBishton @kLabz I didn't want to do it at first, since it'd require a .bind and would make reading the method harder. But seems other methods also use binds - so I've merged them now.

@EricBishton The PR has now shifted to a separate libpath() method, which has a different objective than path().

mastef · 2018-02-08T09:59:18Z

@EricBishton bumped to 3.3.1

back2dos · 2018-02-08T10:11:15Z

Uhm, I suppose the first question is: what do you mean by the "path" of a library? The location where it's on the file system? Because that's not what the first line of haxelib path will tell you. What it will tell you is the class path of the sources of the library. In most libraries, it tends to be src within the library, while in a few others it's indeed the library root, but on occasion it's somethin else like in promhx, where it's src/main. Your enthusiasm is appreciated, but might I propose to first state your goal and then write code? ^^

mastef · 2018-02-08T10:18:44Z

Hi @back2dos this PR follows a discussion on gitter, and the intent was stated in #407 : or a flag could be provided that would only return the path of the library.

Apologies if not everything has been stated here. You're right that the current usage of path is confusing - as it'd be confusing for anybody else reading the documentation. Your edge-case is perfectly valid critique and should be handled.

In this case it looks like haxelib path is more of a haxelib srcpath. And what I was looking for is a haxelib libpath - something you'd expect being returned when using a command called haxelib path <lib> with the vague description give paths to libraries.

mastef · 2018-02-08T10:21:11Z

So I could re-do this and fix the documentation of haxelib path <lib> to give paths to libraries' sources and necessary build definitions, and add a command called haxelib libpath <lib> with returns the root path of a library.

back2dos · 2018-02-08T10:37:08Z

Ok, sorry for the confusion. May I suggest to give slightly more context next time? In a year even those who followed the gitter discussion will have forgotten it, and then the PR will be as enigmatic to everyone, as it has been to me.

As for the rest: as Phillipe stated it's a separate feature. For that reason alone, I'd not interlace it with haxelib path. Also, haxelib path is a very critical part of the client (it's what allows Haxe to resolve libraries) and therefore I think we should try to keep it as simple as possible.

What I would definitely advise is to make sure the logic is exposed for programmatic use, so that when compiling with -lib haxelib_client one can use it directly, rather than passing through the OS. It may be argued that calling haxelib is a better idea, because then you know the result is 100% consistent with haxelib, but on the other hand this means relying on the user having the latest haxelib installed (to actually support this command). In the near future, that's a highly unsafe assumption to make.

mastef · 2018-02-08T10:41:16Z

Yes, well I stated it in the linked issue 😄

@back2dos I've updated the PR now though to be separate from haxelib path. I've undone any changes to the path() logic - and called the new command haxelib libpath.

kLabz · 2018-02-08T11:16:42Z

src/haxelib/client/Main.hx

@@ -1306,7 +1307,7 @@ class Main {
 			if( p.project == prj ) {
 				if( p.version == version )
 					return;
-				throw "Library "+prj+" has two version included "+version+" and "+p.version;
+				throw "Library "+prj+" has two versions included : "+version+" and "+p.version;


Nice catch for the plural :) There is no space before : though

@kLabz What do you mean with There is no space before : though? There's a space before and after the colon : same as in L1297 which is 13 lines above it. ( And L918 )

I meant there should not be a space before :, but as it's written this way all over the project (maybe because haxe has a french history :P), you can just ignore this comment :)

kLabz · 2018-02-08T11:18:08Z

src/haxelib/client/Main.hx

+			var a = args[argcur++].split(":");
+			var results = new List();
+			checkRec(rep, a[0],a[1],results);
+			if( !results.isEmpty() ) Sys.println(results.first().dir);


Why only results.first()?

Well, I suppose the question is again what exactly you want. Assuming you really want just the one path (and not the dependencies), this is fine. But then looping the args is nonsense. Instead you should make sure there's exactly one argument left and process just that.

results.first() will give you the library's root directory, which haxelib libpath should return. Any other paths would be dependency paths, and are discarded by using first().

The looping is replicating the behaviour from haxelib path openfl lime - if you run with haxelib libpath openfl lime it will return the paths for both libraries.

If you only run it with haxelib libpath openfl you will only get one path.

Since one path method works that way, I didn't want to make it confusing by changing the behaviour of the libpath method. The end-user doesn't have to supply multiple arguments.

mastef · 2018-02-08T13:29:36Z

@back2dos I want to be very clear on what this command should accomplish :

After we run haxelib install openfl
haxelib libpath openfl should return:
/usr/local/lib/haxe/lib/openfl/7,0,0/

After we run haxelib git openfl https://github.com/openfl/openfl
haxelib libpath openfl should return:
/usr/local/lib/haxe/lib/openfl/git/

After we run haxelib dev openfl /projects/my/openfl
haxelib libpath openfl should return:
/projects/my/openfl/

After we run haxelib set openfl git
haxelib libpath openfl should return:
/usr/local/lib/haxe/lib/openfl/git/

After we run haxelib set openfl 3.6.1
haxelib libpath openfl should return:
/usr/local/lib/haxe/lib/openfl/3,6,1/

Basically the library manager should inform us where the library is currently located - which corresponds to the location where it was installed into.

back2dos · 2018-02-08T14:12:52Z

I see. Well then I'd say it mostly looks good.

One thing to consider is that swallowing dependencies is maybe not a good idea (or there should be a way to opt out at least), because otherwise the caller may have to replicate that logic too.

mastef · 2018-02-08T14:19:13Z

@back2dos I think I know what you mean - I would prefer to not touch any path() logic though, since it's so important. So I'd rather have libpath fail, than having path fail due to a logic change in checkRec. Although it's really just wrapping a conditional around L1315-L1317 the return would still be a list with a list.first() call. So not much is gained I think.

mastef · 2018-02-09T02:22:11Z

@back2dos dependencies are now not returned with the changes of c91c109

EricBishton · 2018-02-09T07:12:45Z

Looks like it will do no harm to existing code paths. I'd say it's good to go.

andyli · 2018-02-13T06:56:29Z

For the version bump, there is no need to (read: don't) do so in PRs. Imagine there are multiple PRs bumping it, there will be conflicts. Also, since we're following semver, adding new feature means bumping "minor" instead of "bugfix". i.e. It should be 3.4.0.

Same for run.n, don't rebuild it in PRs since it will cause merge conflict.

Please add some example outputs in the documentation. It is not obvious what the output format will be when querying multiple libraries - paths can be separated by line-breaks, commas, colons (like $PATH) etc.

Finally, rebase the changes on top of current development to see if CI passes - I will merge if it does.

EricBishton reviewed Feb 8, 2018

View reviewed changes

mastef changed the title ~~Adds haxelib pathonly - Closes #407~~ Adds haxelib libpath - Closes #407 Feb 8, 2018

kLabz reviewed Feb 8, 2018

View reviewed changes

mastef force-pushed the development branch from c91c109 to ee630e3 Compare February 28, 2018 12:36

mastef closed this Feb 28, 2018

mastef force-pushed the development branch from 0a16a7c to 14ef9fd Compare February 28, 2018 12:53

mastef mentioned this pull request Feb 28, 2018

Adds haxelib libpath #410

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds `haxelib libpath` - Closes #407 #408

Adds `haxelib libpath` - Closes #407 #408

mastef commented Feb 8, 2018 •

edited

Loading

EricBishton commented Feb 8, 2018

EricBishton Feb 8, 2018 •

edited

Loading

mastef Feb 8, 2018

mastef Feb 9, 2018 •

edited

Loading

mastef commented Feb 8, 2018

back2dos commented Feb 8, 2018

mastef commented Feb 8, 2018

mastef commented Feb 8, 2018 •

edited

Loading

back2dos commented Feb 8, 2018

mastef commented Feb 8, 2018

kLabz Feb 8, 2018

mastef Feb 8, 2018 •

edited

Loading

kLabz Feb 9, 2018

kLabz Feb 8, 2018

back2dos Feb 8, 2018

mastef Feb 8, 2018 •

edited

Loading

mastef commented Feb 8, 2018 •

edited

Loading

back2dos commented Feb 8, 2018

mastef commented Feb 8, 2018

mastef commented Feb 9, 2018

EricBishton commented Feb 9, 2018

andyli commented Feb 13, 2018

Adds haxelib libpath - Closes #407 #408

Adds haxelib libpath - Closes #407 #408

Conversation

mastef commented Feb 8, 2018 • edited Loading

EricBishton commented Feb 8, 2018

EricBishton Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

mastef Feb 8, 2018

Choose a reason for hiding this comment

mastef Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

mastef commented Feb 8, 2018

back2dos commented Feb 8, 2018

mastef commented Feb 8, 2018

mastef commented Feb 8, 2018 • edited Loading

back2dos commented Feb 8, 2018

mastef commented Feb 8, 2018

kLabz Feb 8, 2018

Choose a reason for hiding this comment

mastef Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

kLabz Feb 9, 2018

Choose a reason for hiding this comment

kLabz Feb 8, 2018

Choose a reason for hiding this comment

back2dos Feb 8, 2018

Choose a reason for hiding this comment

mastef Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

mastef commented Feb 8, 2018 • edited Loading

back2dos commented Feb 8, 2018

mastef commented Feb 8, 2018

mastef commented Feb 9, 2018

EricBishton commented Feb 9, 2018

andyli commented Feb 13, 2018

Adds `haxelib libpath` - Closes #407 #408

Adds `haxelib libpath` - Closes #407 #408

mastef commented Feb 8, 2018 •

edited

Loading

EricBishton Feb 8, 2018 •

edited

Loading

mastef Feb 9, 2018 •

edited

Loading

mastef commented Feb 8, 2018 •

edited

Loading

mastef Feb 8, 2018 •

edited

Loading

mastef Feb 8, 2018 •

edited

Loading

mastef commented Feb 8, 2018 •

edited

Loading