Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

Regexdispatcher #1944

Merged
merged 40 commits into from
Jun 8, 2018
Merged

Conversation

e1528532
Copy link
Contributor

@e1528532 e1528532 commented Apr 27, 2018

Purpose

Calculates regexes for checker plugins for the use with the typechecker, e.g. converting check/range=0-5000 to a regex representation of that range, also supports the enum checks currently.

Checklist

  • commit messages are fine ("module: short statement" syntax and refer to issues)
  • I added unit tests
  • I ran all tests locally and everything went fine
  • affected documentation is fixed
  • I added code comments, logging, and assertions (see doc/CODING.md)
  • meta data is updated (e.g. README.md of plugins)
  • release notes are updated (doc/news/_preparation_next_release.md)

@markus2330 based on the typechecker branch, the question here is how many things i want to support. I'd add the validation keyword as it more or less represents regexes directly but then lets see, i'd like to get back to writing soon. Basically only the src/plugins/regexdispatcher folder is really relevant and it will be easier to see once the prototype is merged to main.

@e1528532 e1528532 force-pushed the regexdispatcher branch 4 times, most recently from 73f5c22 to 183d1c6 Compare May 29, 2018 13:26
@e1528532
Copy link
Contributor Author

e1528532 commented Jun 4, 2018

@ingwinlu is there possible some cache involved in the docker container? Can i look into the workspace? This issue

setup: can't find source for Elektra/PluginProcess in

/home/jenkins/workspace/libelektra_PR-1944-NHTCZWJOQXIAMT5YFRVQPHMSUTCZBFPXHGIAGV2EUFI3EU7I6QNQ@2/src/bindings/haskell/src,

dist/build/autogen

is not reproducible on my local debian, the initial case issue should be fixed with the commit dcf7995.

@ingwinlu
Copy link
Contributor

ingwinlu commented Jun 4, 2018

is there possible some cache involved in the docker container?

No, workspace is cleaned before and after every checkout

Can i look into the workspace?

The workspace is deleted completely for that test case.

This issue is not reproducible on my local debian

It is for me. Always fails.

Double check your last commit.
You have a typo.

Your commit:
haskell/src/Elektra/Pluginprocess.chs → ...ngs/haskell/src/elektra/PluginProcess.chs
The error:
can not find Elektra/PluginProcess

So maybe try renaming elektra/PluginProcess.chs to Elektra/PluginProcess.chs.

@e1528532
Copy link
Contributor Author

e1528532 commented Jun 5, 2018

i see. since i use git on mac that doesn't really differentiate between cases i've totally overlooked that. i guess it was still cached somehow on my debian vm. good catch!

@ingwinlu
Copy link
Contributor

ingwinlu commented Jun 5, 2018

good catch!

I will tell the build system the next time i log in.

@markus2330
Copy link
Contributor

Sorry, we both fixed the same problem. Please rebase.

@e1528532
Copy link
Contributor Author

e1528532 commented Jun 5, 2018

@markus2330 here you go finally

Copy link
Contributor

@markus2330 markus2330 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Nice PR but it is unclear for me what the Regexdispatcher is for!

- infos/recommends =
- infos/placements = presetstorage
- infos/status = maintained experimental
- infos/metadata =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which metadata is used?


## Introduction

A plugin which generates regex-representations of specification keywords for the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Input/output is unclear: which meta data is consumed, which is generated?


A plugin which generates regex-representations of specification keywords for the
typechecker plugin before saving a configuration specification. Intended to be
mounted before the typechecker plugin.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"before" is misleading: the actual order need to be specified with placements or infos/ordering


In order to generate regex representations for different specification keywords, mount a configuration specification along with this plugin.

`kdb mount <specification> spec/<path> <storage plugin to read the specification> regexdispatcher typechecker`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not let the typechecker depend on the regexdispatcher? Or is it usful to use the typechecker without regexdispatcher!

Please describe more clearly why the regexdispatcher is needed.

Copy link
Contributor Author

@e1528532 e1528532 Jun 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i will. basically the typechecker can also work without the regexdispatcher for certain metadata like check/long or check/validation or similar which do not need an intermediate translation process.
However something like check/enum/#1=A, check/enum/#2=B needs to be translated to the regex A|B, which the regexdispatcher handles to keep concerns separated.
Furthermore other people may implement similar plugins that translate their own metakeys to regexes (that don't necessarily have to be implemented in haskell) and so i concluded to keep those things separated from the typechecker plugin.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I assumed something like that. But the docu does not say it.

* cabal, the haskell build system, usually bundled with ghc
* augeas, which provides libfa utilized by this plugin

## Limitations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong section name, you describe features and not limitations here.

* @param plugin a pointer to the plugin
* @param data the pointer to the data
*/
void elektraPluginProcessSetData (Plugin * handle, void * data)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this serialized and sent to the other process?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is not, the proxied plugins just run in the child process and on linux it seems to share the memory between the parent and the child process. Thus if the child sets any plugin data directly, this would override the plugin data of the parent process, causing the pluginprocess library to fail.
Therefore i store it inside the pluginprocess data structure on the children's side to avoid this problem.

@@ -16,6 +16,11 @@
#include <kdbpluginprocess.h>
#include <stdio.h>

// TODO:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for writing the TODO. Having a way to store plugin data is essential.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually i've forgotten to remove this TODO, this is already implemented as i needed it to pass the linux builds.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even better 👍

find_package (fa)

if (LIBFA_FOUND)
add_haskell_plugin (regexdispatcher MODULE "RegexDispatcher" SANDBOX_ADD_SOURCES "src/libs/typesystem/libfa")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how are the tests specified?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests are written in C

--
-- @file
--
-- @brief Some parsers which are used in the translation process
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not clear to me what the parsers are for.

@@ -0,0 +1,36 @@
- infos = Information about the regexdispatcher plugin is in keys below
- infos/author = e1528532 <[email protected]>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please write your name.

@e1528532
Copy link
Contributor Author

e1528532 commented Jun 5, 2018

some very good points you've raised there especially regarding the ordering and the metakeys and the purpose.

doc/METADATA.ini Outdated
usedby/plugins = regexdispatcher
description = the generated regex representing the type of check/enum/# and check/enum/multi metakeys

[elektra/spec/regex/check/range]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some thoughts about the metadata name:

  • the elektra prefix does not give information, all meta data is for and from Elektra.
  • it should also not start with spec (do not be confused with the namespace).

And wouldn't it be better that we only have a single metadata and the regex is always merged together? Do you need the information of where the regex comes from?

What was the idea of giving this name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right. as the regexdispatcher does all the preprocessing this essentially yields a check/validation keyword in the end all the time, so in that case i don't need further information where it came from i think. i will try that idea out.

@e1528532
Copy link
Contributor Author

e1528532 commented Jun 8, 2018

@markus2330 as discussed in #160 these few checks are unlikely to every succeed and thus i'd suggest merging it now? as far as i know they don't check any haskell related things anyway.

@markus2330 markus2330 merged commit 54e7c17 into ElektraInitiative:master Jun 8, 2018
@markus2330
Copy link
Contributor

thank you!

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

Successfully merging this pull request may close these issues.

3 participants