Maven repository package supplier added #1415

andre2007 · 2018-03-17T19:26:17Z

The purpose of this pull request is to be a door opener for dub in commercial sector. In build infrastructure it is in many cases not allowed to connect to the public internet (code.dlang.org) for security reasons. Instead the companies runs their own repositories using software like Nexus or Artifactory.
It is quite hard to convince system administrators to also run additional software like dub registry.

To solve this problem, this pull request enables dub to connect to maven repositories to retrieve dub packages. Environment variable DUB_REGISTRY and console argument --registry now accepts additional protocols (dub/mvn). Example:

--registry dub+https://code.dlang.org
--registry mvn+http://localhost:8040/artifactory/libs-release/dubpackages

These Maven urls consists of the path to a maven repository + a group id.
(libs-release is the maven repository, dubpackages is the group id)

In the maven repository you deploy your dub package with group id "dubpackages" and
as artifactId you use dub package name and as version you use dub package version.

Maven creates a metadata xml file which get quite handy for retrieving metadata needed for dub

<?xml version="1.0" encoding="UTF-8"?>
<metadata modelVersion="1.1.0">
  <groupId>dubpackages</groupId>
  <artifactId>d-unit</artifactId>
  <version>0.8.2</version>
  <versioning>
    <latest>0.8.2</latest>
    <release>0.8.2</release>
    <versions>
      <version>0.8.2</version>
    </versions>
    <lastUpdated>20180317184845</lastUpdated>
  </versioning>
</metadata>

This metadata and the archives (zip)files are stored using canonical paths.
http://localhost:8040/artifactory/libs-release/dubpackages/d-unit/maven-metadata.xml
http://localhost:8040/artifactory/libs-release/dubpackages/d-unit/0.8.2/d-unit-0.8.2.zip

As far as I know Maven does not provide a nice generic way to implement the searchPackages method.
Therefore I left this method empty.

Can you have a look at this pr?

dlang-bot · 2018-03-17T19:26:18Z

Thanks for your pull request and interest in making D better, @andre2007! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

andre2007 · 2018-03-18T13:25:44Z

Test added

s-ludwig · 2018-03-21T12:55:14Z

Didn't review the code in detail, yet, but from the outside everything looks good. Considering that there have been additional proposals for extended repository support, maybe we should start thinking about how to generalize the registry protocol specification, could for example be a prefix (maven,https://...) or part of the schema (maven+https://... or dub+https://...).

andre2007 · 2018-03-21T13:55:43Z

I really like the "maven+https://" proposal. This makes the additional console argument and env variable superfluous. I will adapt the coding.

andre2007 · 2018-03-21T20:12:47Z

The new logic does not work due to bug #1418

andre2007 · 2018-03-22T15:34:15Z

source/dub/dub.d

+
+	Allowed protocols are dub+http(s):// and maven+http(s)://.
+*/
+PackageSupplier getRegistryPackageSupplier(string url)


I worked around bug #1418 by adapting the string before creating URL.
If this is OK I would leave the method as it is and solve the bug in URL in an independent pr.

andre2007 · 2018-03-22T15:36:33Z

What is your preference?

mvn+https://
maven+https://
m2+https://

wilzbach · 2018-03-23T16:28:44Z

What is your preference?

mvn+https://

andre2007 · 2018-03-23T19:50:16Z

I changed to protocol mvn+https.
Ready for review:)

wilzbach

A few comments. Looks nice already!

wilzbach · 2018-03-23T16:41:59Z

source/dub/packagesupplier.d

@@ -290,6 +290,150 @@ class RegistryPackageSupplier : PackageSupplier {
 	}
 }



Not related to this PR, but imho with this file getting larger and larger,this would be better of in being a package.

wilzbach · 2018-03-23T16:43:37Z

source/dub/packagesupplier.d

+			return null;
+		Version[] ret;
+		foreach (json; md["versions"]) {
+			auto cur = Version(cast(string)json["version"]);


.get!string

wilzbach · 2018-03-23T16:43:54Z

source/dub/packagesupplier.d

+			ret ~= cur;
+		}
+		ret.sort();
+		return ret;


This method is still uncovered.

Should be covered now. An additional test was added to issue1416-maven-repo-pkg-supplier.sh which should retrieve the latest version.

wilzbach · 2018-03-23T16:48:47Z

source/dub/packagesupplier.d

+					continue;
+				}
+			}
+		}


I know that the RegistryPackageProvider does this too, but it's probably better to this generically in a RetryingPackageProvider, but that's not something this PR needs or should address.

or a retrying download function

wilzbach · 2018-03-23T16:51:53Z

source/dub/packagesupplier.d

+		URL m_mavenUrl;
+		struct CacheEntry { Json data; SysTime cacheTime; }
+		CacheEntry[string] m_metadataCache;
+		Duration m_maxCacheTime;


In theory you still might run out of memory within one day.

wilzbach · 2018-03-23T16:55:19Z

source/dub/packagesupplier.d

+			m_metadataCache.remove(packageId);
+		}
+
+		auto url = m_mavenUrl~ NativePath(packageId ~ "/maven-metadata.xml");


Nit: you could be more consistent with spacing when using ~ - at least within the same line ;-)

wilzbach · 2018-03-23T16:56:49Z

source/dub/packagesupplier.d

+					continue;
+				}
+			}
+		}


Looks like we are better off with having a "generic" downloadRetry method for this. Could be a simple parameter to download?

wilzbach · 2018-03-23T16:58:35Z

source/dub/packagesupplier.d

+			}
+		}
+
+		Json json = serializeToJson(["name": Json(packageId), "versions": Json.emptyArray]);


auto json = Json(["name": Json(packageId), "versions": Json.emptyArray]);

wilzbach · 2018-03-23T16:58:52Z

source/dub/packagesupplier.d

+		Json best = null;
+		Version bestver;
+		foreach (json; md["versions"]) {
+			auto cur = Version(cast(string)json["version"]);


.get!string

wilzbach · 2018-03-23T17:01:07Z

source/dub/packagesupplier.d

+			} else if (bestver.isPreRelease) {
+				if (!cur.isPreRelease || cur > bestver) best = json;
+			} else if (!cur.isPreRelease && cur > bestver) best = json;
+			bestver = Version(cast(string)best["version"]);


Instead of copy/pasting this from the other supplies,I suggest to move it in a separate method.

wilzbach

Re-added the comments that got outdated while I added them. Stupid GH :/

wilzbach · 2018-03-23T19:59:42Z

source/dub/dub.d

+*/
+PackageSupplier getRegistryPackageSupplier(string url)
+{
+	if (url.toLower.startsWith("dub+"))


avoid the allocation -> asLowerCase

Do we even need lowering?

You could make this generic and faster (startsWith supports a variadic overload for better performance as the string checking is only done once)

import std.algorithm : map; import std.meta : aliasSeqOf; enum needles = [ ["dub+", "RegistryPackageSupplier"], ["maven+", "M2RegistryPackageSupplier"], ]; switch(url.startsWith(aliasSeqOf!(map!(a => a[0])(needles)))) { static foreach (uint i, el; needles) case i + 1: mixin("return new "~ el[1] ~"(URL(url["~el.length.to!string~"0 .. $]));"); default: return new RegistryPackageSupplier(URL(url)); }

edit: you probably can't use static foreach yet as GDC doesn't support this yet, but the "normal" CTFE foreach works as well.

wilzbach · 2018-03-23T20:00:16Z

test/test_maven_registry.d

+	router.get("*", folder.serveStaticFiles);
+	listenHTTP(text(":", port), router);
+	runApplication();
+}


Might be worthwhile to look into whether the duplication from test_registry.d can be avoided.
Maybe a switch to make fallbacks optional?
Though OTOH it might be a good idea to check whether the retry logic actually works.

You are right, I removed this file and use test_registry.d instead

wilzbach · 2018-03-23T20:00:34Z

test/test_maven_registry.d

@@ -0,0 +1,19 @@
+/+dub.sdl:
+dependency "vibe-d" version="~>0.8.3-alpha.1"


0.8.3 has been released for a while now.

By changing to 0.8.3 there are some failing tests:
Root package test_maven_registry reference vibe-d ~>0.8.3 cannot be satisfied.
https://travis-ci.org/dlang/dub/jobs/357730493

wilzbach · 2018-03-23T20:01:06Z

test/issue1416-maven-repo-pkg-supplier.sh

+"$DUB" fetch maven-dubpackage --version=1.0.5 --skip-registry=all --registry=mvn+http://localhost:$PORT/maven/release/dubpackages
+
+if ! dub remove maven-dubpackage --non-interactive --version=1.0.5 2>/dev/null; then
+    die 'DUB not have installed package from maven registry.'


Spelling:

DUB did not install

DUB doesn't have the installed

wilzbach · 2018-03-23T20:01:26Z

source/dub/packagesupplier.d

+	private {
+		URL m_mavenUrl;
+		struct CacheEntry { Json data; SysTime cacheTime; }
+		CacheEntry[string] m_metadataCache;


When only new packages are added this cache is never cleared. It doubt it matters because usage is through one-time CLI mostly + there are less than a million MVN packages.

andre2007 · 2018-03-23T21:10:45Z

Thanks for your comments, the easy ones I already solved, the others will take some time.

The MavenRegistry cache logic is copy paste from RegistryPackageSupplier, the out of memory issue might occurs here too.
I will check whether I can enhance the download method as proposed by you
This will not compile with error: variable i & el not available at compile time. I will have a deeper look

import std.algorithm : map;
import std.meta : aliasSeqOf;
enum needles = [
	["dub+", "RegistryPackageSupplier"],
	["maven+", "M2RegistryPackageSupplier"],
];
switch(url.startsWith(aliasSeqOf!(map!(a => a[0])(needles))))
{
	foreach (uint i, el; needles)
		case i + 1:
			mixin("return new "~ el[1] ~"(URL(url["~el.length.to!string~"0 .. $]));");
	default:
		return new RegistryPackageSupplier(URL(url));
}

wilzbach · 2018-03-23T21:20:04Z

This will not compile with error: variable i & el not available at compile time. I will have a deeper look

Oh I just used static foreach, because I more used to it: https://run.dlang.io/is/c3Axuv
For legacy support with CTFE foreach you need to use an AliasSeq in the foreach, e.g. by aliasSeqOf: https://run.dlang.io/is/mDQ86x

andre2007 · 2018-03-24T14:52:16Z

I tried to generalize the download retry but I didn't found a nice way. By generalizing you loose the dedicated error/log statements:
Error getting metadata for package...
Package %s not found at...
Maven metadata %s not found...

There is one failing test, s.th. about /dub/test/ddox.sh, I am not really sure what is the cause:

etching libdparse 0.7.1 (getting selected version)...
Failed to extract zip archive for libdparse 0.7.1...
Failed to extract zip archive for libdparse 0.7.1...
Failed to extract zip archive for libdparse 0.7.1...
ZipException: no end record
[ERROR] /home/travis/build/dlang/dub/test/ddox.sh:11 command failed
[ERROR] /home/travis/build/dlang/dub/test/ddox.sh:11 command failed
[ERROR] Script failure.

https://travis-ci.org/dlang/dub/jobs/357735491

andre2007 · 2018-03-25T09:55:25Z

Blocked by sporadic dub registry network issues and error in vibed eventcore 0.8.34.
vibe-d/eventcore#74

Failing test cause by:

../../../.dub/packages/eventcore-0.8.34/eventcore/source/eventcore/drivers/posix/driver.d(145,14): 
Error: safe function 'eventcore.drivers.posix.driver.PosixEventDriverCore!(SelectEventLoop, LoopTimeoutTimerDriver, PosixEventDriverEvents!(SelectEventLoop, PosixEventDriverSockets!(SelectEventLoop))).PosixEventDriverCore.dispose' 
cannot call system function 'core.atomic.atomicStore!(cast(MemoryOrder)3, Mutex, typeof(null)).atomicStore'

andre2007 · 2018-03-29T09:59:08Z

@wilzbach
unfortunately the solution with aliasSeqOf does not work on dmd 2.069.2 and 2.068.2:

 Error: module std.meta import 'aliasSeqOf' not found

My proposal is to have a straight forward solution. I will update the pr

andre2007 · 2018-03-29T18:31:27Z

Any chance to get this pr tagged for 1.9.0?

WebFreak001 · 2018-03-29T18:51:19Z

source/dub/dub.d

+	else if (url.startsWith("mvn+"))
+	{
+		return new MavenRegistryPackageSupplier(URL(url[4..$]));
+	}


how about using the startsWith overload which returns a uint with multiple needles here? if this gets more this will probably be a bit ugly because of lots of copy paste, though startsWith might not be the perfect solution here as then you would be limited to a fixed length for all the checks you use there.

Thank you for the tip, I adapted the coding and use multiple needles now

wilzbach · 2018-03-31T07:31:01Z

Any chance to get this pr tagged for 1.9.0?

Done. I don't see any bigger issues, so this should hopefully work out :)

WebFreak001 · 2018-03-31T14:10:32Z

source/dub/packagesupplier.d

+					continue;
+				}
+			}
+		}


or a retrying download function

WebFreak001 · 2018-03-31T14:13:29Z

source/dub/packagesupplier.d

+
+	private Json getMetadata(string packageId)
+	{
+		import std.xml;


not sure if it's a good idea to import this, vibe should really contain something for xml or at least html parsing

I checked, unfortunately vibed doesn't contain anything regarding xml. It would maybe possible to parse the text via regex (Find all child "version" tags within parent tag "versions") or plain string functions. But as phobos contains a xml parsers which works quite well for this scenario, I am not 100% sure.

WebFreak001 · 2018-03-31T14:16:33Z

...ue1416-maven-repo-pkg-supplier/maven/release/dubpackages/maven-dubpackage/maven-metadata.xml

+    <release>1.0.6</release>
+    <versions>
+      <version>1.0.5</version>
+	  <version>1.0.6</version>


mixed tab and space here

andre2007 · 2018-04-18T18:24:59Z

Do you have a time schedule when this pull request can be merged?

wilzbach

I guess anytime. Any objections @s-ludwig?

andre2007 mentioned this pull request Mar 18, 2018

Maven repository package supplier #1416

Closed

andre2007 changed the title ~~WIP: Maven repository package supplier added~~ Maven repository package supplier added Mar 18, 2018

andre2007 changed the title ~~Maven repository package supplier added~~ WIP: Maven repository package supplier added Mar 21, 2018

andre2007 changed the title ~~WIP: Maven repository package supplier added~~ Maven repository package supplier added Mar 21, 2018

andre2007 commented Mar 22, 2018

View reviewed changes

wilzbach mentioned this pull request Mar 23, 2018

[WIP] support dependencies as git url (including with exact commit hash) #1403

Closed

wilzbach reviewed Mar 23, 2018

View reviewed changes

andre2007 changed the title ~~Maven repository package supplier added~~ [Blocked] Maven repository package supplier added Mar 26, 2018

andre2007 mentioned this pull request Mar 28, 2018

Use the more generic 0.8 version for Vibe.d in 'dub init' to avoid it getting outdated too fast #1429

Closed

andre2007 changed the title ~~[Blocked] Maven repository package supplier added~~ Maven repository package supplier added Mar 28, 2018

WebFreak001 reviewed Mar 29, 2018

View reviewed changes

wilzbach added this to the 1.9.0 milestone Mar 31, 2018

WebFreak001 reviewed Mar 31, 2018

View reviewed changes

WebFreak001 approved these changes Mar 31, 2018

View reviewed changes

Maven repository package supplier added

c322acf

wilzbach approved these changes Apr 18, 2018

View reviewed changes

s-ludwig merged commit 24caef7 into dlang:master May 3, 2018

		@@ -290,6 +290,150 @@ class RegistryPackageSupplier : PackageSupplier {
		}
		}

		@@ -0,0 +1,19 @@
		/+dub.sdl:
		dependency "vibe-d" version="~>0.8.3-alpha.1"

Maven repository package supplier added #1415

Maven repository package supplier added #1415

Conversation

andre2007 commented Mar 17, 2018 • edited Loading

dlang-bot commented Mar 17, 2018

andre2007 commented Mar 18, 2018

s-ludwig commented Mar 21, 2018

andre2007 commented Mar 21, 2018 via email

andre2007 commented Mar 21, 2018

Choose a reason for hiding this comment

andre2007 commented Mar 22, 2018

wilzbach commented Mar 23, 2018

andre2007 commented Mar 23, 2018

wilzbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andre2007 commented Mar 23, 2018

wilzbach commented Mar 23, 2018 • edited Loading

andre2007 commented Mar 24, 2018 • edited Loading

andre2007 commented Mar 25, 2018 • edited Loading

andre2007 commented Mar 29, 2018

andre2007 commented Mar 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach commented Mar 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andre2007 commented Apr 18, 2018

wilzbach left a comment

Choose a reason for hiding this comment

andre2007 commented Mar 17, 2018 •

edited

Loading

wilzbach commented Mar 23, 2018 •

edited

Loading

andre2007 commented Mar 24, 2018 •

edited

Loading

andre2007 commented Mar 25, 2018 •

edited

Loading