Skip to content

Conversation

@javihernandez
Copy link
Member

This pull request includes a first device reporter plugin, which uses the packageKit node module.
As a brief explanation, the deviceReporter plugin is being initialized at start-up time and stores all the installed packages in memory, which is used later on in order to provide a quick response.

Currently, and as a demonstration of how it works, this pull request includes the bits on the solution registry to use this new functionality.

To try this new feature, you have to start de the GPII in production mode, this is:

NODE_ENV="fm.ps.sr.dr.mm.os.lms.production" node gpii.js

sgithens and others added 28 commits March 17, 2014 20:23
[changes]
  * Removing old example code
  * Adding searchPackage, installPackage and removePackage methods
  * Updating tests
[changes]
  * Drop wscript
  * Include binding.gyp
add-on -- added method updatePackage().  Updated unit tests as
appropriate.
files, and, generally, improved the output to make it more apparent
what was found.  Removed commented out code (stderr output).
packagekit's "search files" function that returns package
package information on the path(s).
Also added utility handleFiltersArgumen() that is shared by
searchFiles() and searchPackage().
to make use of the new searchFiles() function of the packagekit
add-on.
It now bases the search on what the "find" command returned, not on the input to the find command.
Also, improved the name of the array containing the paths found by the find command.
This change fixes a segfault in openSUSE, which is not reproducible in
Fedora.
The getPackages() function takes a filter as input and returns a
list of packages that match that filter.  For example, the filter
'~installed', results in a list of all the packages not installed
but available.

- added getPackages() to nodepackagekit.cc.
- modified internal handleFiltersArgument() to take an index arg.
- added unit test for getPackages().
- modified the updatePackage() unit test.
- generally cleaned up unit tests.
Added getPackages() function to nodepackagekit add-on
* Adding required index.js and package.json files
* Adding a get method to the Device Reporter implementation; get
  method returns the following structure:

  {
    "provider": "device reporter id"
    "data": "available data (pkgs)"
  }
This patch should fix the random segmentation fault when using the
packagekit's node module.
Copy link
Member

Choose a reason for hiding this comment

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

No need for this constant, in JavaScript we believe strings are good enough to be constants

.jshintrc Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Please update this version for current trunk

Copy link
Member

Choose a reason for hiding this comment

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

Somehow this code was changed without addressing the original comment - these repeated blocks should be separated into dedicated functions

Copy link
Member

Choose a reason for hiding this comment

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

I thought I had addressed the comment. I looked for commonalities and attempted to refactor those into a common function and leaves the variable parts as is.

Please give specifics: which blocks (line numbers)? Please suggest a signature for this common function.

Copy link
Member

Choose a reason for hiding this comment

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

The lines 179 through 181 duplicate the flow of the lines 193 through 195. The lines 183 through 185 duplicate the flow of the lines 189 through 191. These should be consolidated using functions of the form
pkTests.testRemovePackage(id, matchPkg)
pkTests.testInstallPackage(id, matchPkg)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks.

Thinking out loud here...

The lines 179 through 181 duplicate the flow of the lines 193 through 195.
I"m not sure of that: what is passed as the second parameter to pkTests.testRemovePackage() -- matchPkg? That structure has to be initialized after the remove operation since the information it holds changes due to the remove (and/or install) action. There's nothing to pass in. Or maybe sometimes it's null and other times it is a useful value.

I'll think about it some more.

@javihernandez
Copy link
Member Author

@amb26, ready (again)

@sgithens
Copy link
Member

@javihernandez @amb26 Now that we've merged in the fixes for acceptance tests on linux, can these be tested and merged? Are there any more changes to be made for the actual code of this ticket. (ie. if it still works we can merge it in now )

@javihernandez
Copy link
Member Author

@amb26 , @sgithens this is ready again after pushing the last three commits to match the latest changes on master. ;)

Copy link
Member

Choose a reason for hiding this comment

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

Hi @javihernandez,
There is a call to g_new() in nodepackagekit.cc, namely, in the function utf8StringFromValue(). But, there isn't any corresponding g_free() when the code is done with that memory. See:
https://developer.gnome.org/glib/unstable/glib-Memory-Allocation.html#glib-Memory-Allocation.description

Copy link
Member

Choose a reason for hiding this comment

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

@javihernandez.
Followup: or, is it because the g_char buffer is stored in a C++ string, that it automatically gets g_free()ed when the C++ string goes out of scope?

Copy link
Member Author

Choose a reason for hiding this comment

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

@klown,
yeah, that is the expected behavior. IIRC that was something that Antranig asked me to do during our meet in Crete.

@kaspermarkus
Copy link
Member

The fate of this JIRA will depend on the decision made in http://piratepad.net/c4a-arch-28-01-2015

@klown
Copy link
Member

klown commented Jan 28, 2015

@kaspermarkus Also relevant is the merge of the fire ball code into @javihernandez's device-reporter-draft2 branch: see javihernandez#16

javihernandez and others added 3 commits February 6, 2015 16:03
…raft-2-update

* upstream/master: (22 commits)
  GPII-1041: Updated the .jshintrc entry and jshinted everything
  GPII-1032:  Modified uninstallUsbLib grunt task (linux).
  GPII-1027: Adding a TODO in relation to GPII-1028
  GPII-1026: Fixing the implementation of XRandR's 'get' method
  GPII-1026: Enable XRandR's tests in UnitTests.sh
  GPII-1026: Removing support for XRandR's brightness setting
  GPII-1027: Fix ALSA's implementation of get method
  GPII-1025:  Fixed typo.
  GPII-1025:  Move creation of GPII log directory to grunt task.
  GPII-434: pushd requires an argument
  GPII-434: Second attempt at Unit Tests driver - gsettings test must be run from its own directory
  GPII-434: Added top-level driver for all settings handler tests (with XRANDR commented out)
  GPII-1001: Handle more than just one solution entries
  GPII-434: Notation on broken tests
  GPII-434: Added driver file and readme omitted from previous commit
  GPII-1001: Testing the full get/set flow with non-exixtent values
  GPII-1001: Avoid the use of JSON.stringify to strip the content values out
  GPII-1001: Updating orca's settings handler tests
  GPII-1001: Changing the way Orca settings handler operated
  GPII-920: Modify USB lib grunt tasks to handle non-fatal errors gracefully.
  ...

Conflicts:
	Gruntfile.js
	gpii.js

Manually fixed:
        index.js
- gpii.js and index.js:  moved the fluid.require()s of the node
add-ons, including the one for packagekit (device reporter) from
gpii.js to index.js,

- gpii/node_modules/packagekit/test/all-tests.js: now uses
kettle.loadTestingSupport() and removes kettle.test.allTests boolean,

- tests/UnitTests.sh: added the packagekit node add-on tests.
As per v1.0.3 of PackageKit, added pkTests.availableNotInstalled()
to check if the package list returned using the "~installed"
filter includes an already installed package, and ignores that
package if found.  Used by the "search" and "get" tests.

This is backward compatible with the way the previous version of
PackageKit worked (v0.8.17), so the tests continue to work with
that version as well.
@sgithens sgithens merged commit b131fde into GPII:master Jul 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants