ibm-capi / pslse

Power Service Layer Simulation Engine
28 stars 22 forks source link

no default for PSLSE compile after merging PSL8 and PSL9 #99

Closed joergkayser closed 6 years ago

joergkayser commented 6 years ago

When compiling the master branch of PSLSE, we see compile errors in libcxl. The QUICKSTART notice shows new environment variables PSL8 and PSL9 to be set, but there is no information in README. Also according to QUICKSTART, the PSL8 and PSL9 should not be set both to 1. This cries for a better solution

and finally (unrelated to the above)

fhaverkamp commented 6 years ago

Hi,

By this change several test-cases which automatically clone pslse:master broke. This might not affect us alone but also other users doing this type of testing. I suggest that if changes are done, they should be done if possible in a backwards compatible manor. Also setting and environment variable before the compile and letting the compile break in the middle if it is not set is not optimal. If you do not get around those env variables, you could add some preprocessor checks and print a more meaningful message e.g.

#ifndef CONFIG_BLABLA
#error "Please set CONFIG_BLABLA before you start compiling this code"
#endif 

Best would be of course dropping to a default which ensures backwards compatibility. I support what Joerg said before, introducing multiple env vars with conflict potential is not a good idea.

Thanks

Frank

fhaverkamp commented 6 years ago

I got a complaint message as follows:

Hallo Frank,
... 
ibcxl.c: In function '_psl_loop':
libcxl.c:1127: warning: unused variable 'op2'
libcxl.c:1127: warning: unused variable 'op1'
libcxl.c:1123: warning: unused variable 'function_code'
libcxl.c:1123: warning: unused variable 'op_size'
libcxl.c: In function '_pslse_connect':
libcxl.c:1499: error: 'PSLSE_VERSION_MAJOR' undeclared (first use in this function)
libcxl.c:1499: error: (Each undeclared identifier is reported only once
libcxl.c:1499: error: for each function it appears in.)
libcxl.c:1500: error: 'PSLSE_VERSION_MINOR' undeclared (first use in this function)
make: *** [libcxl.o] Error 1

Mit freundlichen Grüßen / Kind regards

And that is not my code ...

LanceThompson commented 6 years ago

Please use release tag v3.1 to avoid this issue for the time being

joergkayser commented 6 years ago

@LanceThompson we went back to tag=v3.1, but we also see, that you made changes in master to revert the last weeks change. But master still needs the variable PSL8=1 in order to succesfully compile. Uma made changes in branch=capi2 to have one variable PSLVER=8|9 instead of two. She also added a msg in case this variable is not set. Both is not implemented in master or not reverted back to a state, where no variable is needed.

As our customers need a clear directive, please tell us, what you plan for branch=master, and when this change is available

LanceThompson commented 6 years ago

Hi @joergkayser . We're going to leave master as is for the next 2 week approximately, if that is ok. We are all going to be out on holiday soon and just felt it would be better to leave it stable. As I mentioned, to avoid all the churn, remain on tag v3.1. When I tried to revert the merge, it didn't appear to work, so I didn't want make matters worse. If you want to keep using the new code, you can use the capi2 branch. In January, we will merge capi2 into master again and it will have the single env var PSLVER.

joergkayser commented 6 years ago

Thanks. I notify our users to take v3.1 for the moment and keep this ticket open, until master works again

joergkayser commented 6 years ago

@LanceThompson Please state (for our customers as well), how much longer we should stay on v3.1 for POWER8 applications. For a merged functionality on the master branch we need to document the additional environment variable PSLVER=8/9 or notify the user to stay on tag=v3.1, one or the other. Please advise

joergkayser commented 6 years ago

We are getting asked from our users again and again, how long they have to stay on v3.1 instead of working with the master branch. Please provide a new schedule. Thanks

LanceThompson commented 6 years ago

We merged capi2 into master (again) a week or so ago. It seems like it has been ok. Is it ok to close this?

joergkayser commented 6 years ago

I am running fine with master on both P8 and P9 platforms. Thanks