Added new maven-targets option to maven plugin. #627

Closed
wants to merge 9 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

ZenHarbinger commented Jul 3, 2016

Composite maven projects do not build to 'target'. This update allows the user to specify the child target to copy.

parts:
  algolink-mvn:
    plugin: maven
    source: .
    maven-targets:
      - algolink-gui
      - algolink-cli

By default, the basedir/target directory is searched, otherwise it uses the 'target' directory of each specified value.
The installdir for each child project is the name of the child project. If no values are specified, it reverts to the default of 'jar' or 'war' depending on the target type of the project.

Added new maven-targets option to maven plugin.
Composite maven projects do not build to 'target'. This update allows the user to specify the child target to copy.

parts:
  algolink-mvn:
    plugin: maven
    source: .
    maven-targets:
      - algolink-gui
      - algolink-cli
By default, the basedir/target directory is searched, otherwise it uses the 'target' directory of each specified value.
The installdir for each child project is the name of the child project. If no values are specified, it reverts to the default of 'jar' or 'war' depending on the target type of the project.

Can one of the admins verify this patch?

Can one of the admins verify this patch?

ZenHarbinger added some commits Jul 3, 2016

Fixed too long of a line.
Fixed unused warfiles reference.
Collaborator

sergiusens commented Jul 4, 2016

ok to test

Collaborator

sergiusens commented Jul 7, 2016

👍 from me

Can one of the admins verify this patch?

Can one of the admins verify this patch?

Collaborator

sergiusens commented Jul 7, 2016

ok to test

+ raise RuntimeError("could not find any"
+ "built jar files for part")
+ basedir = 'jar' if jarfiles and len(f) == 0 else (
+ 'war' if warfiles and len(f) == 0 else f)
@kyrofa

kyrofa Jul 7, 2016

Member

I personally think this is difficult to read. I'd prefer a normal if.

@ZenHarbinger

ZenHarbinger Jul 7, 2016

Contributor

I get that, but then I couldn't pass the coverage and complexity.

@kyrofa

kyrofa Jul 7, 2016

Member

Perhaps a refactor should be considered, then.

@kyrofa

kyrofa Jul 7, 2016

Member

Indeed, this isn't tested at all. Definitely missing some tests, there.

@ZenHarbinger

ZenHarbinger Jul 7, 2016

Contributor

I have no idea how the dummy testing works, or how to set up a composite maven project for that. I'm willing to do it though.

@kyrofa

kyrofa Jul 7, 2016

Member

Yes please-- that's how we ensure this 1) works correctly, and 2) doesn't break in the future 😃 .

@ZenHarbinger

ZenHarbinger Jul 7, 2016

Contributor

Where can I find any docs on this, it was pretty rough to understand what was there to begin with.

@kyrofa

kyrofa Jul 7, 2016

Member

I'm afraid there are none. You have a good start-- you added the maven_targets to the Options class there, but you need to write some tests that actually make use of them and verify the results. This test is probably the simplest place to start, learn from, and build upon.

+
+ targetdir = os.path.join(self.installdir, basedir)
+ os.makedirs(targetdir, exist_ok=True)
+ self.run(['cp', '-a'] + arfiles + [targetdir])
@kyrofa

kyrofa Jul 7, 2016

Member

I'd prefer to use the python utilities for copying files rather than shelling out.

@ZenHarbinger

ZenHarbinger Jul 7, 2016

Contributor

Sure, that's just what was already there.

@sergiusens

sergiusens Jul 7, 2016

Collaborator

El 07/07/16 a las 16:35, Kyle Fazzari escribió:

In snapcraft/plugins/maven.py
#627 (comment):

  •    for f in self.options.maven_targets:
    
  •        src = os.path.join(self.builddir, f, 'target')
    
  •        jarfiles = glob.glob(os.path.join(src, '*.jar'))
    
  •        warfiles = glob.glob(os.path.join(src, '*.war'))
    
  •        arfiles = glob.glob(os.path.join(src, '*.[jw]ar'))
    
  •        if not (arfiles):
    
  •            raise RuntimeError("could not find any"
    
  •                               "built jar files for part")
    
  •        basedir = 'jar' if jarfiles and len(f) == 0 else (
    
  •                  'war' if warfiles and len(f) == 0 else f)
    
  •        targetdir = os.path.join(self.installdir, basedir)
    
  •        os.makedirs(targetdir, exist_ok=True)
    
  •        self.run(['cp', '-a'] + arfiles + [targetdir])
    

I'd prefer to use the python utilities for copying files rather than
shelling out.

Ah, nice catch!


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/snapcore/snapcraft/pull/627/files/c8dc9a6288c6c68c11da4f5c45e3e4b49493bae0#r69972186,
or mute the thread
https://github.com/notifications/unsubscribe/ABF_5QIHLRzqaA1js3VMss5evoyqw9icks5qTVUbgaJpZM4JD7dL.

@sergiusens

sergiusens Jul 7, 2016

Collaborator

El 07/07/16 a las 16:37, Matthew Aguirre escribió:

In snapcraft/plugins/maven.py
#627 (comment):

  •    for f in self.options.maven_targets:
    
  •        src = os.path.join(self.builddir, f, 'target')
    
  •        jarfiles = glob.glob(os.path.join(src, '*.jar'))
    
  •        warfiles = glob.glob(os.path.join(src, '*.war'))
    
  •        arfiles = glob.glob(os.path.join(src, '*.[jw]ar'))
    
  •        if not (arfiles):
    
  •            raise RuntimeError("could not find any"
    
  •                               "built jar files for part")
    
  •        basedir = 'jar' if jarfiles and len(f) == 0 else (
    
  •                  'war' if warfiles and len(f) == 0 else f)
    
  •        targetdir = os.path.join(self.installdir, basedir)
    
  •        os.makedirs(targetdir, exist_ok=True)
    
  •        self.run(['cp', '-a'] + arfiles + [targetdir])
    

Sure, that's just what was already there.

@kyrofa pass the secret sauce on copytree link_or_copy ;-)

@kyrofa

kyrofa Jul 7, 2016

Member

@kyrofa pass the secret sauce on copytree link_or_copy ;-)

Ah, good idea.

Instead of copying this, you can hard-link them (and copy as a backup) using something like this.

@ZenHarbinger

ZenHarbinger Jul 7, 2016

Contributor

OK, I just checked in a fix for this, I will work on the tests later.
Use of: shutil.copy(); we don't want to copy the tree of the target directory because it would grab all the intermediary output too, not just the jar or war file.

Member

kyrofa commented Jul 7, 2016

This is looking good, thank you! A little more work to do. Also, please make sure that:

  1. A bug exists for the feature you're trying to add, and
  2. You sign the contributor license agreement.

More details about both of these items can be found in the contribution guide.

Contributor

ZenHarbinger commented Jul 7, 2016

Who is: Canonical Project Manager or contact for the contributor agreement?

@ZenHarbinger ZenHarbinger deleted the ZenHarbinger:features/maven-target branch Jul 7, 2016

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