[IN_PROGRESS][JENKINS-18583] - Fixed PerforceSCM instatination forn Multiple SCMs

[oleg-nenashev]

Hello Robert,

Just for review…
Proposed fix resolves issue with instantiation of Perforce plugin in the MultipleSCM’s hetero list (JENKINS-18583). Issue was in the incomplete handling of data in the DataBoundConstructor.

If fix is applicable, I’ll fix tests and remove test stuff from the *.pom. Then, we will be able to merge pull request.

Best regards,
Oleg Nenashev
R&D Engineer, Synopsys Inc.
www.synopsys.com

[buildhive]

Jenkins » perforce-plugin #95 FAILURE
Looks like there's a problem with this pull request
(what's this?)

[buildhive]

Jenkins » perforce-plugin #96 FAILURE
Looks like there's a problem with this pull request
(what's this?)

[buildhive]

Jenkins » perforce-plugin #97 FAILURE
Looks like there's a problem with this pull request
(what's this?)

[jenkinsadmin]

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

[rpetti]

Was the change to the core version necessary? I'd like to maintain backwards compatibility with Hudson if possible.

[rpetti]

Also, shouldn't the DepotType class be in the same package as the rest of the plugin?

[oleg-nenashev]

Hello Robert,

Actual state of the PR is here: https://issues.jenkins-ci.org/browse/JENKINS-18583

At current state, PR allows to run only one p4 instance with other SCMs. I haven't finished implementation of any-case solution yet, so we can't merge this request to the incoming version. So, please consider PR as a review request.

Regarding your question:

• Version can be returned to the initial state. I use newer version in order to have MultipleSCMs plugin for testing. I'll restore version before completion of PR.
• I agree that it would be better to place DepotType at the hudson.plugins.perforce.${?}. I'll check applicability of such approach in case of our open-source contribution policies.

Probably, I shouldn't create PRs with unfinished functionality in future.

[oleg-nenashev]

Probably, we could use "in-progress" milestone to differentiate statuses of the pull-requests

[rpetti]

Sounds good, thanks!

[buildhive]

Jenkins » perforce-plugin #101 FAILURE
Looks like there's a problem with this pull request
(what's this?)

[oleg-nenashev]

Hello,

Finally, hack for the radioBlock groups was obvious. Now perforce plugin works well in the Multiple SCMs wrapper.
Seems that we can revert changes in pom-file and merge it into the main branch for testing.

BR, Oleg Nenashev

[oleg-nenashev]

To be reverted...

[rpetti]

Getting a few failed tests after reverting the pom changes. Round trip config tests are failing with

java.lang.NullPointerException
    at hudson.plugins.perforce.PerforceSCM$PerforceSCMDescriptor.newInstance(PerforceSCM.java:1793)
    at hudson.plugins.perforce.PerforceSCM$PerforceSCMDescriptor.newInstance(PerforceSCM.java:1769)

Which seems to relate to retrieving the p4.depotType parameter in the newInstance method. Since the config.xml has been changed to use ${instanceID}.depotType, shouldn't that be changed in newInstance as well? The same will probably need to be done with all the variables used in newInstance, as I believe this problem extends beyond the depotType field.

[buildhive]

Jenkins » perforce-plugin #103 FAILURE
Looks like there's a problem with this pull request
(what's this?)

[oleg-nenashev]

-- Round trip config tests are failing with NPE
Hmm. Seems that I haven't reconfigured jobs w/o MultipleSCMs in my tests. Sorry for wasting your time, I should check PRs more carefully in the future. I'll fix newInstance methods ASAP. BTW, we should consider usage of Data-bound constructor in the future (to prevent code duplication)

-- The same will probably need to be done with all the variables used in newInstance, as I believe this problem extends beyond the depotType field
I suppose that issue it related to radioBlock fields only, because multiple instantiations of Perforce SCMs use same group. Anyway, I'll check propagation of "Advanced" options

[rpetti]

The reason these fields use the newInstance method of configuration is because they don't work in the databound constructor. These fields are hidden in optional blocks, and thus not always available to stapler. If a configuration is posted to the databound constructor with missing fields, an exception is thrown back into the user's face and the config is not saved.

I suppose there might be a way to use separate classes for these optional configuration sections with their own databound constructors and configs, but I haven't investigated that possibility.

[oleg-nenashev]

I've evaluated possible approaches with indexing instead of p4 prefixes. BTW, every change leads to ugly code.
I'll implement additional classes like it has been done for depotType.

[buildhive]

Jenkins » perforce-plugin #114 FAILURE
Looks like there's a problem with this pull request
(what's this?)

[buildhive]

Jenkins » perforce-plugin #115 FAILURE
Looks like there's a problem with this pull request
(what's this?)

[buildhive]

Jenkins » perforce-plugin #116 FAILURE
Looks like there's a problem with this pull request
(what's this?)

[oleg-nenashev]

Robert, I've implemented draft implementation, which does not utilize newInstance() hack.

I propose to corrupt PerforceSCM's interface and remove setter methods to reduce code size. AFAIK, these methods aren't used outside Perforce plugin. Anyway, external usage is not secure, because setters don't invoke save() ...

[rpetti]

Setters are used by some other plugins, so I don't believe they should be removed.

I'll need to take a closer look at your implementation, since I was fairly certain that stapler would fail if you were missing a field (as would be the case with the optional arguments). I'm curious to see how you got around this!

Can you please explain what you mean by "propose to corrupt PerforceSCM's interface"? That sounds like a bad thing to do... We need to maintain backwards compatibility, and provide seamless upgrades.

[oleg-nenashev]

Can you please explain what you mean by "propose to corrupt
PerforceSCM's interface"?

This statement is related to setters only. If there is no external
dependencies, we could remove these methods in order to decrease code size
and prevent potential usage errors. Plugin doesn't save configuration after
invocation of setter => it may lead to difference from disk configuration.

However, we have no choice if setters are being used by external plugins.

2013/10/3 Rob Petti notifications@github.com

Setters are used by some other plugins, so I don't believe they should be
removed.

I'll need to take a closer look at your implementation, since I was fairly
certain that stapler would fail if you were missing a field (as would be
the case with the optional arguments). I'm curious to see how you got
around this!

Can you please explain what you mean by "propose to corrupt PerforceSCM's
interface"? That sounds like a bad thing to do... We need to maintain
backwards compatibility, and provide seamless upgrades.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/33#issuecomment-25589068
.

[oleg-nenashev]

In the latest changes I've corrupted the initial RadioButtons patch. :(
BTW, new databound constructor works well.

[cloudbees-pull-request-builder]

plugins » perforce-plugin #52 UNSTABLE
Looks like there's a problem with this pull request

[cloudbees-pull-request-builder]

plugins » perforce-plugin #60 FAILURE
Looks like there's a problem with this pull request

[oleg-nenashev]

Seems I've forgot about this PR :(
@rpetti
Robert, have you found some time to take a look on the approach? If it is applicable, I'll restore the fix for radio-button conflicts and update the data to simplify the merge.

[rpetti]

I thought this was still incomplete? At least that's what it looks like.

The use of DepotType is confusing to me... It seems to contain fields for all 3 different depot types, rather than splitting it out into different implementations of DepotType (ie, StreamDepotType, ClientFileDepotType, ViewDepotType).

There's also no config.jelly to be found for configuring DepotType objects, so I'm not sure how this should be working without running into the missing field exceptions I described earlier.

Finally, the PerforceSCM config.jelly doesn't seem to be using these new objects at all, so if you could save the config without immediately getting an exception, the fields still wouldn't be getting saved.

[oleg-nenashev]

Rob, the current implementation aims the initial structure of plugin data to avoid readResolve() and other such stuff till the refactoring of Perforce plugin. So the change just relies on Jenkins core, which constructs classes using JSON data bounding. All data will be saved to initial fields of PerforceSCM classes, so there should not be any data conflicts.

I use the change for several months without any issues on restart.

[rpetti]

So what is the purpose of DepotType if it's not being used by the configuration? That's probably a better question.

I don't see how configuration would work at all with the code being the way it is. You cannot have missing fields without databinding to an instanced subclass that has different fields (such as what is being done with the Browsers), but that appears to be exactly what is happening here.

[oleg-nenashev]

newInstance()

1. DepotType and other classes are being used in @DataBoundConstructor parameters. It is enough to specify the data for Stapler's runtime.
2. After that, the data will be saved to proper fields of PerforceSCM class
3. After that, the new class instance will be serialized to XML file

load():

1. The data will be de-serialized to existing PerforceSCM class via XStream
2. DataBoundConstructor and my classes are not required

So the approach should work properly. BTW, I agree that fields should be moved to proposed classes during the refactoring

[rpetti]

DepotType doesn't have a config.jelly, and PerforceSCM's jelly isn't using DepotType, so I don't see how this could work as it is currently.

[oleg-nenashev]

http://stapler.kohsuke.org/apidocs/org/kohsuke/stapler/DataBoundConstructor.html

[rpetti]

I still do not see how this code would work.

1. You have added the DepotType object and depotType field to PerforceSCM's databound constructor. This is fine, but you have not changed the radioBlock to use it (it still just has 'name="p4.depotType"', which can only be accessed from the raw request using newInstance, thus breaking multiple instantiation).
2. The DepotType class contains three fields that are all mutually exclusive (thanks to the radioBlock). Were the config.jelly posted, there would be two of three fields missing from the request, resulting in an exception and an unsaved config.

Am I mistaken?

[oleg-nenashev]

1. Stapler truncates name of the field, so it creates depotType object, which will be passed to constructor
2. Missing fields will become nulls in DepotType constructor. After that, class methods will these nulls

In 1.480.x and 1.509.x this code works properly without any exceptions. It also allows to avoid newInstance() issues in 1.536+

[rpetti]

1. So I take that to mean that stapler automatically spiders through subclass DataBound constructors looking for matching fields, even if the view isn't for that class?
2. Missing fields had always caused exceptions for me whenever I've tried them, which is why newInstance was needed in order to handle fields within optional blocks. If this isn't the case, then I really, really messed up.

It seems that I don't have a good handle on this, so I'd say just proceed in whatever way you see fit.

[oleg-nenashev]

Any optional block creates a new hierarchy level in JSON data, so you cannot handle its data without creating of nested structures like DepotType. On the other hand, bindJSON() just parses input JSON tree and does not care about initial jelly files at all.

BTW, there's a one significant exception: non-nullable fields (integer, boolean, etc.). These fields must present in input JSON if your constructor declares them in parameters. Probably, you've had problems due to such types.

[rpetti]

Ok. If you want to resolve the conflicts and merge it, then I'd say go right ahead. I tested it separately, and it seems to work fine.

On the multiple-scms front, I think there might be an issue with the PerforceTagAction, since there's only one per build and you could conceivably have multiple changesets and servers to tag in a single build when using multi-scm. The email resolver will probably stop working as well, since it looks for projects that use PerforceSCM directly, not ones nested in multiple-scms.

[cloudbees-pull-request-builder]

plugins » perforce-plugin #70 FAILURE
Looks like there's a problem with this pull request

[cloudbees-pull-request-builder]

plugins » perforce-plugin #72 SUCCESS
This pull request looks good

[oleg-nenashev]

@rpetti
Robert, the PR is ready for the merge.
"useViewMaskForChangeLog" field has been ported to the new structure.

I've also merged previous commits into one to simplify analysis in future.

[oleg-nenashev]

@rpetti
Robert, when are you going to release a new version of Perforce plugin?.
The current master branch contains many useful changes.

[rpetti]

Sorry, I've been on vacation, and my workstation was recently wiped. I'll
deploy it to my system and release it after some testing.
On Jan 9, 2014 2:45 AM, "Oleg Nenashev" notifications@github.com wrote:

@rpetti https://github.com/rpetti
Robert, when are you going to release a new version of Perforce plugin?.
The current master branch contains many useful changes.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/33#issuecomment-31915738
.