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

HPCC-17060 Changes to ecl standard library not included in package #9598

Merged

Conversation

Projects
None yet
5 participants
@richardkchapman
Copy link
Member

richardkchapman commented Feb 10, 2017

Signed-off-by: Richard Chapman rchapman@hpccsystems.com

Type of change:

  • This change is a bug fix (non-breaking change which fixes an issue).
  • This change is a new feature (non-breaking change which adds functionality).
  • This change is a breaking change (fix or feature that will cause existing behavior to change).
  • This change alters the query API (existing queries will have to be recompiled)

Checklist:

  • My code follows the code style of this project.
    • My code does not create any new warnings from compiler, build system, or lint.
  • The commit message is properly formatted and free of typos.
    • The commit message title makes sense in a changelog, by itself.
    • The commit is signed.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly, or...
    • I have created a JIRA ticket to update the documentation.
    • Any new interfaces or exported functions are appropriately commented.
  • I have read the CONTRIBUTORS document.
  • The change has been fully tested:
    • I have added tests to cover my changes.
    • All new and existing tests passed.
    • I have checked that this change does not introduce memory leaks.
    • I have used Valgrind or similar tools to check for potential issues.
  • I have given due consideration to all of the following potential concerns:
    • Scalability
    • Performance
    • Security
    • Thread-safety
    • Premature optimization
    • Existing deployed queries will not be broken
    • This change fixes the problem, not just the symptom
    • The target branch of this pull request is appropriate for such a change.
  • There are no similar instances of the same problem that should be addressed
    • I have addressed them here
    • I have raised JIRA issues to address them separately

Testing:

Built a package, modified standard library, built again, observed the change was taken.

@hpcc-jirabot

This comment has been minimized.

Copy link

hpcc-jirabot commented Feb 10, 2017

@richardkchapman

This comment has been minimized.

Copy link
Member Author

richardkchapman commented Feb 10, 2017

@Michael-Gardner Please review

@Michael-Gardner

This comment has been minimized.

Copy link
Contributor

Michael-Gardner commented Feb 10, 2017

@richardkchapman There is a small bug with this (at least on my 16.04 environment).

The gpg command won't overwrite the ${CMAKE_CURRENT_BINARY_DIR}/${module} file when it goes to update the file, which causes a failure. If you add the line...

COMMAND bash "-c" "rm -f ${CMAKE_CURRENT_BINARY_DIR}/${module}"

add_custom_command just before the GPG_COMMAND_STRING, it should take care of the issue.

@richardkchapman

This comment has been minimized.

Copy link
Member Author

richardkchapman commented Feb 10, 2017

@Michael-Gardner That sounds like a separate bug rather than a problem with this fix...

Will that work if the commands get run in parallel as I think was discovered recently?

@richardkchapman

This comment has been minimized.

Copy link
Member Author

richardkchapman commented Feb 10, 2017

A bit of googling suggests adding --batch --yes to the gpg command might help

@Michael-Gardner

This comment has been minimized.

Copy link
Contributor

Michael-Gardner commented Feb 10, 2017

I'll test the batch --yes option now

@Michael-Gardner

This comment has been minimized.

Copy link
Contributor

Michael-Gardner commented Feb 10, 2017

@richardkchapman adding --yes after the --batch in the gpg command will work

@richardkchapman

This comment has been minimized.

Copy link
Member Author

richardkchapman commented Feb 10, 2017

@Michael-Gardner Is that everywhere that --batch is used, or just in this macro?

@Michael-Gardner

This comment has been minimized.

Copy link
Contributor

Michael-Gardner commented Feb 10, 2017

We really should only need it in the Macro. Looking at the code for the 'gpg signing check' that happens on our cmake call, I don't think we'd gain anything by adding it there. If I was to re-write that section to use an existing file as our target for checking whether or not we could do --clearsign, then having the --yes option would cut down on some code.

HPCC-17060 Changes to ecl standard library not included in package
Signed-off-by: Richard Chapman <rchapman@hpccsystems.com>

@richardkchapman richardkchapman force-pushed the richardkchapman:ecl-std-depends branch from fbbcef8 to 5f61b32 Feb 13, 2017

@richardkchapman

This comment has been minimized.

Copy link
Member Author

richardkchapman commented Feb 13, 2017

@Michael-Gardner Repushed to include the extra --yes

@HPCCSmoketest

This comment has been minimized.

Copy link
Contributor

HPCCSmoketest commented Feb 13, 2017

Automated Smoketest:
Sha: 5f61b32
Build: success
ECL Watch: Rebuilding Site

errors warnings build time
0 65 91.514 seconds

Install hpccsystems-platform-community_6.2.7-closedown0.el7.x86_64.rpm
HPCC Start: OK

Unit tests result:

Test total passed failed errors timeout
unittest 88 88 0 0 0
wutoolTest(Dali) 19 19 0 0 0

HPCC Stop: OK
HPCC Uninstall: OK

@Michael-Gardner

This comment has been minimized.

Copy link
Contributor

Michael-Gardner commented Feb 15, 2017

@richardkchapman Tested and works on Ubuntu 16.04. Looks good.

@ghalliday ghalliday merged commit 95ac976 into hpcc-systems:candidate-6.2.8 Feb 15, 2017

@richardkchapman richardkchapman deleted the richardkchapman:ecl-std-depends branch Jul 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.