Evaluate Architecure #231

Tomschi · 2017-02-02T20:31:58Z

Add static field for hardware architecure.

coveralls · 2017-02-02T20:34:55Z

Coverage increased (+0.0007%) to 94.456% when pulling 13df6b0 on Tomschi:master into ab25f67 on apache:master.

kinow · 2017-02-12T04:42:37Z

I remember a discussion about it some time ago.

The issue with this approach was that os.arch tells only the JVM arch, not really OS arch.

If there is a strong use case for these properties, we would have to be careful with the javadocs. Right now the javadocs don't explicitly tell the user that it is the JVM arch, and not the OS arch.

Other approaches involve reflection or JNI, to find out the OS arch, but I think that is a bit too tricky to get it done correctly.

PascalSchumacher · 2017-02-12T09:47:17Z

The discussion mentioned by @kinow is here: https://issues.apache.org/jira/browse/LANG-1145

coveralls · 2017-02-13T06:51:55Z

Coverage decreased (-0.006%) to 94.53% when pulling a5945e7 on Tomschi:master into b715d18 on apache:master.

Tomschi · 2017-02-13T06:55:47Z

Rewrite javadoc's.

kinow · 2017-02-13T07:53:11Z

Thanks for updating the pull request @Tomschi.

I don't have a use case for this. I can see where it could be used, but I don't have any project where I would use it. Code is clear, javadocs have been updated :-)

So will leave it here for others to review, comment, and maybe perhaps.

Tomschi · 2017-02-13T08:13:48Z

I'am using it for the jacob-project. There i have to know, if it is a 32 or 64 bit architecture to load the correct dll library.

kinow · 2017-02-13T08:19:06Z

Gotcha, found this example https://github.com/Tomschi/jacob-parent/blob/ec3f1c10169c26f14ee1f61bd6622c67a73e26fc/jacob/src/main/java/com/jacob/com/LibraryLoader.java#L202

Looks like a valid use case. I'm all right with merging it, my +0. Let's now wait to see what others think about it.

Thanks!

PascalSchumacher · 2017-02-14T20:25:52Z

I think this is a worthy addition.

In my experience people often do not read documentation. Maybe we should use IS_32_BIT_JVM so there can be no confusion? Or is this is too paranoid? What do you think?

kinow · 2017-02-14T20:39:40Z

Oh, good point @PascalSchumacher have no objection to it. We could probably avoid a few misunderstandings that way. Happy with that too @Tomschi ?

Tomschi · 2017-02-15T08:13:06Z

It's ok for me, i will change it.

…IT_JVM

coveralls · 2017-02-17T21:05:05Z

Coverage increased (+0.001%) to 94.53% when pulling 3e335fd on Tomschi:master into 30c85ad on apache:master.

PascalSchumacher · 2017-02-20T22:24:00Z

jira issue: https://issues.apache.org/jira/browse/LANG-1313

I plan to merge this tomorrow (if there are no objections).

michael-o · 2017-02-20T22:48:50Z

What is the real pupose for this actually? The client should not care about the arch at all. The regex match is brittle. This will likely fail on ARM and it fails here on Itanium with HP-UX for os.arch IA64N which is a 32 bit VM. I would not add something like this to Commons.

kinow · 2017-02-21T00:25:43Z

@michael-o

What is the real pupose for this actually? The client should not care about the arch at all.

I think @Tomschi use case is valid, where a client could need to know the arch before loading a certain library, and we had another issue submitted LANG-1145 with similar requirement.

The regex match is brittle. This will likely fail on ARM and it fails here on Itanium with HP-UX for os.arch IA64N which is a 32 bit VM.

Note taken, perhaps before merging we can try to cover more archs, like this list:

http://lopica.sourceforge.net/os.html

There is another place where arch is used within Commons too:

https://github.com/apache/commons-crypto/blob/158be0644c353a617427ab190a4f09082cda42ac/src/main/java/org/apache/commons/crypto/OsInfo.java#L28

We can possibly look at how crypto is using it, and if we could maybe use a similar approach here.

michael-o · 2017-02-21T09:38:56Z

@kinow The cheapest way is to produce two bundles, one for 32 bit and one for 64 bit, if possible. The lopica source is useless, it is 15 years old. Commons Crypto is better. hawtjni on GitHub has decent detection code. It is work checking. But the current approach is way too simple.

Create class ArchUtils, which defines methods for evaluating the JVM architecture. Create test class for ArchUtils.

kinow · 2017-03-07T21:00:09Z

Looks better now @Tomschi did you use Crypto's code as reference? cc @michael-o

Tomschi · 2017-03-07T21:04:54Z

Yes, the base of my code references to the commons crypto lib.

kinow · 2017-03-07T21:11:11Z

Excellent @Tomschi

I'm dropping a note to the mailing list to ask for feedback from crypto devs, as there could be some synergy.

I will play with the code at home, but one thing that I noticed is that it seems to contain tabs... minor nit pick, but could you check checkstyle and make sure the code is not introducing any warnings / errors there, please? That way it will be easier to merge the PR.

Thanks!
B

Tomschi · 2017-03-07T21:18:59Z

I'm sorry. I removed the tabs. Style should be ok now.

sebbASF · 2017-03-07T23:23:34Z

src/main/java/org/apache/commons/lang3/ArchUtils.java

+
+    static {
+        map = new HashMap<>();
+


HashMap is not thread-safe.
Since there are public methods which can also update the map, this means the class is not thread-safe either.

Indeed. The crypto implementation doesn't allow changes to the map after it has been created. I think we can use a ConcurrentHashMap, or simply follow what was done in crypto.

Tomschi · 2017-03-10T12:01:49Z

There's no point in having the isXXX() methods any more.
There just need to be methods to get the Processor.
If necessary, the Processor class could have isxxx methods on it.

This methods are for me an easy and null safe way for evaluating. Further, this methods are the reason, why i have done this pull request.

sebbASF · 2017-03-10T19:14:30Z

src/main/java/org/apache/commons/lang3/ArchUtils.java

-    private static final void addProcessor(Processor processor) throws UnsupportedOperationException {
-        if (!map.containsKey(processor.getName())) {
-            map.put(processor.getName(), processor);
+    private static final void addProcessor(String key, Processor processor) throws UnsupportedOperationException {


If the parameters were changed to (Processor processor, String key ...) there would be no need to create the List and unpack it.

sebbASF · 2017-03-10T19:35:30Z

In which case, I think they belong on the Processor class.

The user code would look like:

Processor proc = ArchUtils.getProcessor([String]);
if (proc != null) {
    if (proc.isX86() && proc.isPPC()) {
    }
} // else not supported

At present the same code is

if (ArchUtils.isSupported([String])) {
    if (ArchUtils.isX86([String]) && ArchUtils.isPPC([String]) )
    }
}

I think that looks more complicated.
Also all the isXXX methods have to be overloaded with an optional string parameter.
And the map is accessed at least 3 times.

sebbASF · 2017-03-10T19:38:48Z

src/main/java/org/apache/commons/lang3/ArchUtils.java

+            return ProcessorArch.BIT_32.equals(processor.getProcessorArch());
+        } else {
+            return false;
+        }


The code could be simplified:

Processor processor = map.get(value); return processor != null && ProcessorArch.BIT_32.equals(processor.getProcessorArch());

Similarly for other related methods

coveralls · 2017-03-12T09:12:07Z

Coverage decreased (-0.02%) to 94.513% when pulling 85ea6ce on Tomschi:master into d43e1d0 on apache:master.

Tomschi · 2017-03-15T07:19:26Z

@sebbASF I refactored the classes. Do you have any suggestions, or is it ok?

sebbASF · 2017-03-16T00:02:25Z

src/main/java/org/apache/commons/lang3/ArchUtils.java

+            map.put(key, processor);
+        } else {
+            String msg = "Key " + key + " already exists in processor map";
+            throw new UnsupportedOperationException(msg);


IllegalStateException might be better here

sebbASF · 2017-03-16T00:09:21Z

I thought the idea was to move the isXXX methods to the Processor class.

You could then say

Processor processor = ArchUtils.getProcessor();
processor.is32Bit();
processor.isPPC();
etc.

Tomschi · 2017-03-20T06:32:48Z

I'm not sure, if we really need the isXXX methods in the Processor class, because i can directly equal the enums.

coveralls · 2017-03-20T07:09:32Z

Coverage increased (+0.02%) to 94.589% when pulling dbdb045 on Tomschi:master into 5d6c176 on apache:master.

coveralls · 2017-03-20T07:09:32Z

Coverage decreased (-0.02%) to 94.552% when pulling dbdb045 on Tomschi:master into 5d6c176 on apache:master.

sebbASF · 2017-03-20T10:39:43Z

src/test/java/org/apache/commons/lang3/ArchUtilsTest.java

+        assertNotNull(ArchUtils.getProcessor(X86));
+        assertNull(ArchUtils.getProcessor("NA"));
+    }
+


There are no tests for Processor type.

sebbASF · 2017-03-20T10:48:47Z

Agreed it's not necessary to have the isXXX methods.
However it makes the user code shorter and simpler.

Currently the code is:

Processor processor = ArchUtils.getProcessor();
if (ProcessorArch.BIT_32.equals(processor.getProcessorArch())) {}
if (ProcessorType.PPC.equals(processor.getProcessorType())) {}

It would become:

Processor processor = ArchUtils.getProcessor();
if (processor.is32Bit()) {}
if (processor.isPPC()) {}

As well as being longer, in the first case one has to remember to whether to use ProcessorArch or ProcessorType

sebbASF · 2017-03-20T10:50:49Z

Also it occurs to me that ProcessorArch and ProcessorType could perhaps be subtypes of Processor (Arch and Type). They don't really have an independent existence, so do they need separate classes?

coveralls · 2017-03-21T07:27:42Z

Coverage decreased (-0.009%) to 94.559% when pulling a9c6f59 on Tomschi:master into 5d6c176 on apache:master.

coveralls · 2017-03-21T07:27:42Z

Coverage decreased (-0.009%) to 94.559% when pulling a9c6f59 on Tomschi:master into 5d6c176 on apache:master.

…. Add and refactor tests.

coveralls · 2017-03-22T07:31:15Z

Coverage increased (+0.03%) to 94.597% when pulling 29941f4 on Tomschi:master into 5d6c176 on apache:master.

coveralls · 2017-03-22T07:31:15Z

Coverage increased (+0.03%) to 94.597% when pulling 29941f4 on Tomschi:master into 5d6c176 on apache:master.

PascalSchumacher · 2017-03-26T18:03:35Z

@sebbASF What do you think about the latest changes? Is this pull request ready for merging?

Thanks, Pascal

sebbASF · 2017-03-26T19:42:49Z

looks good

PascalSchumacher · 2017-03-27T18:42:20Z

Merged. Thanks!

Add boolean variables for evaluating architecture.

13df6b0

Tomschi added 2 commits February 13, 2017 07:25

Merge branch 'master' of github:apache/commons-lang

8e0739a

#LANG-1145 Refactor javadoc's

a5945e7

Tomschi added 2 commits February 17, 2017 08:12

Merge branch 'master' of github:apache/commons-lang

ba13fec

#LANG-1145 rename IS_32_BIT to IS_32_BIT_JVM and IS_64_BIT to IS_64_B…

3e335fd

…IT_JVM

Tomschi added 3 commits March 6, 2017 20:48

Merge branch 'master' of github:apache/commons-lang

bfb7090

#LANG-1145 remove fields IS_32_BIT_JVM and IS_64_BIT_JVM

994616c

#LANG-1145 create utility class for evaluating jvm architecture

3522c09

Create class ArchUtils, which defines methods for evaluating the JVM architecture. Create test class for ArchUtils.

Change tabs to 4 whitespaces

6eb918e

sebbASF reviewed Mar 7, 2017

View reviewed changes

#LANG-1145 remove name field in Processor.java

1ead276

sebbASF reviewed Mar 10, 2017

View reviewed changes

#LANG-1145 add licences and remove is methods.

85ea6ce

sebbASF reviewed Mar 16, 2017

View reviewed changes

#LANG-1145 change UnsupportedOperationException to IllegalStateException

ee9c51b

Tomschi added 2 commits March 20, 2017 07:36

Merge branch 'master' of github:apache/commons-lang

d6b325c

#LANG-1145 fix build errors

dbdb045

sebbASF reviewed Mar 20, 2017

View reviewed changes

Tomschi added 2 commits March 21, 2017 08:22

#LANG-1313 move enums to Processor class. Add test cases.

3c70174

#LANG-1313 remove wildcard import

a9c6f59

#LANG-1313 Extend Processor.java with isXXX methods for Arch and Type…

29941f4

…. Add and refactor tests.

asfgit closed this in 90f0a68 Mar 27, 2017

Evaluate Architecure #231

Evaluate Architecure #231

Conversation

Tomschi commented Feb 2, 2017

coveralls commented Feb 2, 2017 • edited

kinow commented Feb 12, 2017

PascalSchumacher commented Feb 12, 2017

coveralls commented Feb 13, 2017 • edited

Tomschi commented Feb 13, 2017

kinow commented Feb 13, 2017

Tomschi commented Feb 13, 2017

kinow commented Feb 13, 2017

PascalSchumacher commented Feb 14, 2017

kinow commented Feb 14, 2017

Tomschi commented Feb 15, 2017

coveralls commented Feb 17, 2017 • edited

PascalSchumacher commented Feb 20, 2017

michael-o commented Feb 20, 2017 • edited

kinow commented Feb 21, 2017

michael-o commented Feb 21, 2017

kinow commented Mar 7, 2017

Tomschi commented Mar 7, 2017

kinow commented Mar 7, 2017

Tomschi commented Mar 7, 2017

sebbASF Mar 7, 2017

Choose a reason for hiding this comment

kinow Mar 8, 2017

Choose a reason for hiding this comment

Tomschi commented Mar 10, 2017

sebbASF Mar 10, 2017

Choose a reason for hiding this comment

sebbASF commented Mar 10, 2017

sebbASF Mar 10, 2017

Choose a reason for hiding this comment

coveralls commented Mar 12, 2017 • edited

Tomschi commented Mar 15, 2017

sebbASF Mar 16, 2017

Choose a reason for hiding this comment

sebbASF commented Mar 16, 2017

Tomschi commented Mar 20, 2017

coveralls commented Mar 20, 2017

coveralls commented Mar 20, 2017 • edited

sebbASF Mar 20, 2017

Choose a reason for hiding this comment

sebbASF commented Mar 20, 2017

sebbASF commented Mar 20, 2017

coveralls commented Mar 21, 2017

coveralls commented Mar 21, 2017

coveralls commented Mar 22, 2017

coveralls commented Mar 22, 2017

PascalSchumacher commented Mar 26, 2017

sebbASF commented Mar 26, 2017

PascalSchumacher commented Mar 27, 2017

coveralls commented Feb 2, 2017 •

edited

coveralls commented Feb 13, 2017 •

edited

coveralls commented Feb 17, 2017 •

edited

michael-o commented Feb 20, 2017 •

edited

coveralls commented Mar 12, 2017 •

edited

coveralls commented Mar 20, 2017 •

edited