Conversation

The old "nodeset" class has some deficiencies documented in #1495. We created libflux-idset as an external, standalone library to export nodeset-like functionality to framework projects, but only created the bare minimum needed by flux-sched at the time and left nodeset in place.

In the spriit of recent API cleanup efforts, this PR finishes the job, adding functionality to idset, converting users, and removing nodeset.

This comment has been minimized.

73f7634: it would be slightly cleaner to add libidset to libflux_internal in a separate commit. Lumping it in with the mrpc update seems arbitrary and isn't symmetrical with the other code that is updated. (or maybe it is? I would also accept a comment in the commit message that describes why libidset was moved into libflux_internal in this commit)

e6b2e84 could use a commit message. The commit subject says the linkage was "fixed" but there isn't a hint why (which could be helpful for future developer wondering why that test cant use $(test_ldadd) like all the other tests)

This comment has been minimized.

Let me see if I can get the idset coverage up just a bit before this gets merged.

I also feel like the way everything is being linked is a little convoluted and might be improved if the veb stuff moved to libidset (though I've tried that twice now and got myself in the weeds both times).

Make the following changes to libidset in preparation
for adding more code:
- namespace the include guard symbol with FLUX_
- create idset_private.h
- add common validate_idset_flags() function
- make idset_set() a public function
- move idset_decode() to idset_decode.c
- move idset_encode() to idset_encode.c
- drop internal magic for user-after-free
- rename ENCODE_CHUNK to IDSET_ENCODE_CHUNK
- use default size on idset_create (size=0)
Note on idset_create():
idset_create (0, IDSET_FLAG_AUTOGROW) is similar to
nodeset_create(), in that it creates an idset with a
default internal size of 1K that is grown to fit id's
>= size as they are set with idset_set ().
idset_create (N, IDSET_FLAG_AUTOGROW), where N > 0, is
similar to nodeset_create_size (N).
idset_create (0, IDSET_FLAG_AUTOGROW), followed by
idset_set () is the equivalent to nodeset_create_rank ().
Similarly, nodeset_create_range () is replaced with
idset_create () followed by idset_set_range ().
(idset_set_range() will be added in a future commit).
Note on idset_encode():
idset_encode (idset, IDSET_FLAG_BRACKETS, IDSET_FLAG_RANGE)
is similar to nodeset_string (ns), except that the returned
string must be freed by the caller, and the flags to the
encode function, rather than settings on the nodeset,
determine whether the idset is encoded with brackets or ranges.

This comment has been minimized.

Rebased on current master and squashed down the unit test addition. I think this could be ready to go in. @chu11, sounds like @grondo may be offline for a bit. Would you mind giving this a quick review?

This comment has been minimized.

minor thought, would it be a net win to do some obvious checks before iterating through everything below? I know vebsize() is costly and perhaps not worth it. T.M != T.M? I don't know the library that well, so maybe it's not a net win.

Problem: as parts of flux in src/modules and src/cmd are
converted from nodset to libidset, they require linkage
with libidset. However, since libidset is dependent
on libutil and parts of libutil are dependent on libidset,
merely adding libidset to LDADD creates a circular dependency.
Add libidset.la to libflux_internal, since the linker
can then resolve dependencies between objects in any
order.

Problem: the libutil/cronodate unit test needs to be
linked against libutil and libidset, but libidset needs
libutil/veb, so merely adding libidset to $(test_ldadd)
creates a circular dependency.
Make a special LDADD for this test program that specifies
the minimum set of objects, in the proper order to satisfy
dependencies.

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.