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

Fix to #53, better fix for #41, and other improvements #82

Merged
merged 8 commits into from
Nov 22, 2013
Merged

Fix to #53, better fix for #41, and other improvements #82

merged 8 commits into from
Nov 22, 2013

Conversation

cunei
Copy link

@cunei cunei commented Nov 22, 2013

This pull request adds several improvements. The first is a fix to #53: the repeatable config is once again repeatable, in the sense that it can be copied and pasted as-is into a new file, and used to reproduce the original build. That fix addresses two main aspects. The first is that the config file now contains a GeneralOptions section, the content of which does not directly impact on the build process (it is used for notifications, deploying, and so on). Because of that, it was not saved as part of the repeatable build, but it is actually necessary in order to reproduce the full syntax of a legal configuration file. A second aspect is that the Repeatable configuration that was being saved relied on ProjectConfigAndExtracted, which using the new schema-based JSON parsing are not exactly a valid replacement for regular project descriptions.

Now, a new full DBuildConfiguration record is created, containing all the information required to repeat the build exactly (including notifications, etc.). In the process, significant improvements have also been made to the solution of issue #41. The previous approach (using variables under "vars.dbuild") had some drawbacks, since it made "Vars" meaningful even after parsing (resolvers would have to be saved in the repeatable config), and also caused parts of the "vars" JSON subtree to be treated specially.

Instead, the resolvers are now handled as a map, as part of the GeneralOptions. The syntax remains basically the same, but vars are no longer required. The sections Vars and Properties in the dbuild config file can be emptied after parsing, while still having a repeatable config in the end.

The documentation concerning using a custom list of resolvers has been completely rewritten, and it is now much clearer.

As a further addition to this pull request, dbuild should now also be able to support (if projects really insist on using it) the macro-paradise plugin, or indeed continuations, or other plugins.

Still concerning repeatability, the Ivy build system will now issue a warning if a repeatable config, saved from a previous run, contains a reference to a -SNAPSHOT, but the resolved version changed since the original run.

Closes #53, and improves the solution to #41

Antonio Cunei added 8 commits November 22, 2013 04:52
In theory this should offer support to projects that rely
on macro-paradise, continuations, or other plugins.
That allows the Vars section, and in particular the subtree
"vars.dbuild", to no longer be treated specially. This also
prepares to creating a new Repeatable config that includes
just the global options (and not any repositories defined
among the vars).
The root of the configuration file has been for a while
now no longer a DistributedBuildConfig, but rather a
DBuildConfiguration, which also includes the GeneralOptions.

Although the latter have no influence on the repeatability
of the build, saving only the RepeatableDistributedBuild
is insufficient, as users won't be able to use it as-is
to repeat the build. Further, a RepeatableDistributedBuild
includes ProjectConfigAndExtracted objects, and with the
schema strongly enforced those won't do in any case as
a legal dbuild configuration file.

This commit restores user repeatability of builds, by
creating a new DistributedBuildConfig after extraction,
which can now again be used as-is to repeat the build.

In the process, added warnings to Ivy's SNAPSHOT
resolutions, so that if the snapshot targets change
a warning is issued.
Will now prefer the main jar, if it exists,
rather than extracting the expanded name of
an artifact at random.
@ghost ghost assigned jsuereth Nov 22, 2013
@jsuereth
Copy link
Contributor

LGTM - except for the explanation of use cases in jenkins. I think we should unify on the launcher's mechanism of specifying repositories because we can re-use that for sbt launched builds and resolving dbuild itself. I agree that you need the feature to specify new repos for general builds, but I think we should clarify that *proxy repositories should be configured via the standard sbt-launcher mechanism: http://www.scala-sbt.org/release/docs/Detailed-Topics/Proxy-Repositories.html#sbt-configuration

@cunei
Copy link
Author

cunei commented Nov 22, 2013

I am aware about the possiblity of sharing the setting between the sbt launcher and dbuild; I actually implemented this functionality in 0a6b400. The point of this addition is indeed offering an option to separate them instead, should that be necessary. Consider the following. The dbuild.properties may not be readily modifiable by the end user, in case the tool is installed by an admin in a shared location.

Think about bash, and the difference between /etc/bash.bashrc and ~/.bashrc. The first comes from the default distribution as-is, or it may be customized system-wide by the administrator in order to offer sensible defaults. The second is per-user. The distinction here is the same: the default dbuild.properties might be shared, and set up by a Jenkins admin: therefore, it may not be easily customizable by the users that set up individual jobs. Using "options.resolvers" offers a way for them to override the hopefully sensible shared defaults, for each individual job. This is exactly the reason why Iulian originally filed the enhancement request; I hope you see why this is useful.

I can hopefully address your comment by making it clearer in the docs that dbuild.properties is system-wide and should be used to set sensible system-wide defaults, including proxies. I will do so shortly.

I will take out of this pull request the #41-related commits, so that the PR can be merged to fix #53 in the meantime. We can continue the discussion on #41 on the more specific pull request for wip-issue-41.

@jsuereth
Copy link
Contributor

Ok, we discussed this in person. Code looks great. Unifying on the proxy mechanism for sbt-launcher is the ideal, but I agree this mechanism is useful and can exist on its own.

jsuereth added a commit that referenced this pull request Nov 22, 2013
Fix to #53, better fix for #41, and other improvements
@jsuereth jsuereth merged commit 9f3e156 into lightbend-labs:master Nov 22, 2013
@jsuereth jsuereth deleted the wip-issue-53 branch November 22, 2013 16:13
@adriaanm
Copy link
Contributor

👍 -- nice!

@adriaanm
Copy link
Contributor

I'll catch up with the resolvers config as soon as this is released.

@cunei
Copy link
Author

cunei commented Nov 29, 2013

dbuild 0.7.1-M1 is available, includes the resolvers code as well as other improvements:
https://github.com/cunei/distributed-build/compare/typesafehub:v0.7.0...v0.7.1-M1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Repeatable build is no longer repeatable build config
3 participants