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

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

Closed
joergkayser opened this issue Dec 13, 2017 · 10 comments
Closed

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

joergkayser opened this issue Dec 13, 2017 · 10 comments

Comments

@joergkayser
Copy link

joergkayser commented Dec 13, 2017

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

  • a hint in Makefile, if compile fails due to variables not set
  • or a default, if the user specifies no environment
  • one environment variable (PSL = P8 / P9) to prevent invalid settings
  • a hint in README to read QUICKSTART

and finally (unrelated to the above)

  • one Makefile in PSLSE_ROOT to call the subdirectory Makefiles in afu_driver/src, libcxl, debug and pslse
@fhaverkamp
Copy link
Contributor

fhaverkamp commented Dec 13, 2017

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
Copy link
Contributor

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
Copy link
Contributor

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

@joergkayser
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Author

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

@joergkayser
Copy link
Author

@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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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

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

No branches or pull requests

3 participants