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

QFJ-855 #31

Merged
merged 10 commits into from Mar 14, 2016
Merged

QFJ-855 #31

merged 10 commits into from Mar 14, 2016

Conversation

manureno
Copy link
Contributor

Hi,
Can you please review this pull request intended to fix the corresponding jira ticket.
POMs of message modules have been updated to use classes generated once by the message-all to produce the jars of message-fix99.

@manureno
Copy link
Contributor Author

Hi OSGi packaging has been removed by mistake, please wait until it is re-integrated

@chrjohn
Copy link
Member

chrjohn commented Feb 13, 2016

Hi @manureno ,
is the missing OSGi packaging the only problem? I.e. if I re-add it, will the problem of QFJ-855 be resolved? Or are there other open points?

Thanks,
Chris.

@manureno
Copy link
Contributor Author

Hi Christoph,

Sorry I've not worked on the subject since my last comment.

As fas as I can remember, this was indeed the only issue detected so far.

I think I changed the packaging strategy from "bundle" to "jar, since the
customization of source directory used by the build helper plugin was not
taken into account when using "bundle".

Packaging strategy
From : bundle
To : jar

Source directory customization
build-helper-maven-plugin
..

${project.basedir}/../quickfixj-messages-all/target/generated-sources/

     </sources>

Sorry again, these is not 100% certified, maybe you can try to revert the
packaging from jar to bundle to check if it re-integrates the osgi meta
data while still producing the proper source packaging.

I'll try to get back on it, let me know

cheers
Emmanuel

On Sat, Feb 13, 2016 at 10:50 PM, Christoph John notifications@github.com
wrote:

Hi @manureno https://github.com/manureno ,
is the missing OSGi packaging the only problem? I.e. if I re-add it, will
the problem of QFJ-855 be resolved? Or are there other open points?

Thanks,
Chris.


Reply to this email directly or view it on GitHub
#31 (comment).

@manureno
Copy link
Contributor Author

manureno commented Mar 6, 2016

Hi Christoph,

Commits up to e024476 should fix the OSGI regression.

Can you please have a look at the pull request with 2 warnings :

  • The tests don't fully pass on my environment, this is true for both the master branch after a clean checkout and for the QFJ-855 branch. The tests in error/failure look nevertheless similar ; please run a clean build of QFJ-855 branch on your environment before merging to check for potential issue.
  • I'm not fully familiar with OSGI, and checked only visually that the manifests of the quickfixj messages bundles looked the same as before, a more thorough test would be better.

Please let me know if this is ok or if some things should be changed

Cheers
Emmanuel

@chrjohn
Copy link
Member

chrjohn commented Mar 7, 2016

Hi Emmanuel,

thanks for the update. For me the build succeeds. :)
What errors did you get? Sometimes there are intermittent errors around the SSL tests and/or SingleThreadedEventStrategy. But these should be resolved very soon.

I am not fully familiar with the original problem, so do you happen to know a simple test case which I can execute to check if QFJ-855 is fixed?

Thanks in advance and cheers,
Christoph.

@manureno
Copy link
Contributor Author

manureno commented Mar 8, 2016

Indeed, the failures are intermittent and linked to SSL.

The ticket is aimed at fixing the support of multiple version of Fix specs in the same process as well as the "VerifyError" that may happen when using the core with individual fix specification jars.
A sample test case is
{code}
public class Fix40MessagesAndCoreClassesCompatibilityTest extends TestCase {
public void testFIX40() {
quickfix.fix40.MessageFactory fact = new quickfix.fix40.MessageFactory();
Object newMessage = fact.create("FIX.4.0", "D");
assertNotNull(newMessage);
}
}
{code}
this JUnit test will throw the "VerifyError" if executed with core jar apearing before fix40 jar in the classpath.
I couldn't yet integrate this into maven/surefire with a proper control on the classpath, any idea is welcome.
cheers

@chrjohn
Copy link
Member

chrjohn commented Mar 8, 2016

Thanks. So you are saying this test should pass after your changes and should fail without your changes (if core jar is before other message jars in the classpath)?

@manureno
Copy link
Contributor Author

manureno commented Mar 8, 2016

yes, that's it

@chrjohn
Copy link
Member

chrjohn commented Mar 8, 2016

OK, thanks. I will check if I can come up with a way to test this via maven.

@chrjohn
Copy link
Member

chrjohn commented Mar 8, 2016

Hi Emmanuel,
I have another question. You have changed the configuration for the bundle plugin to be as follows:

<Export-Package>quickfix.field;version="${project.version}";uses:=quickfix,quickfix.fix50;version="${project.version}";uses:=quickfix,quickfix.field;version="${project.version}"</Export-Package>
<Import-Package>quickfix,quickfix.field;version="${project.version}"</Import-Package>
<Require-Capability>osgi.ee;filter:="(&#38;(osgi.ee=JavaSE)(version=${jdkLevel}))"</Require-Capability>

I was under the assumption that these entries in the manifest are generated automatically by the bundle plugin.
Is there a specific reason why you put these entries in manually?

Thanks and cheers,
Chris.

@manureno
Copy link
Contributor Author

manureno commented Mar 8, 2016

Hi,

The default behavior of the bundle plugin which automatically generates the
manifest didn't take into account the following block :

org.codehaus.mojo
build-helper-maven-plugin
1.9.1


prepare-package
add-source
add-source


${project.basedir}/../quickfixj-messages-all/target/generated-sources/

</sources>

Using the "manifest" goal of the bundle plugin is thus a workaround to
allow the custom source folder declared in the build helper configuration
to be taken into account (it's also a bit faster BTW).

Emmanuel

On Tue, Mar 8, 2016 at 3:56 PM, Christoph John notifications@github.com
wrote:

Hi Emmanuel,
I have another question. You have changed the configuration for the bundle
plugin to be as follows:

quickfix.field;version="${project.version}";uses:=quickfix,quickfix.fix50;version="${project.version}";uses:=quickfix,quickfix.field;version="${project.version}"
quickfix,quickfix.field;version="${project.version}"
osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=${jdkLevel}))"

I was under the assumption that these entries in the manifest are
generated automatically by the bundle plugin.
Is there a specific reason why you put these entries in manually?

Thanks and cheers,
Chris.


Reply to this email directly or view it on GitHub
#31 (comment).

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

Successfully merging this pull request may close these issues.

None yet

2 participants