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

Dependency-related fixes #3

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Dependency-related fixes #3

wants to merge 3 commits into from

Conversation

jsiwek
Copy link
Contributor

@jsiwek jsiwek commented Sep 20, 2017

i.e. related to the following issues:
https://github.com/bro/package-manager/issues/14
https://github.com/bro/package-manager/issues/15
#1

I think the changes in this PR are the way dependencies should be handled by this package. At least for the current state of Bro's master vs. 2.5.1.

@JustinAzoff
Copy link
Contributor

Hmm.. I'm not sure if that's right.. the issue is that the script needs

commit 7c107f9f02abebffbb625a4c76ea62eed8363dca
Merge: ff2c5b934 ff998dfa4
Author: Seth Hall <[email protected]>
Date:   Fri May 12 15:31:32 2017 -0400

    Merge remote-tracking branch 'origin/topic/johanna/notice-suppression'

    * origin/topic/johanna/notice-suppression:
      Lessen cluster node of notice suppression.

Which was added in 2.5-140

if I depend on bro >= 2.5-140 I think it will complain that bro 2.5.1 is not new enough, but if I depend on 2.5.1, it won't allow master.

@jsiwek
Copy link
Contributor Author

jsiwek commented Sep 20, 2017

Did you see I had !=2.5.1 in the version requirement? i.e. If you choose to merge the commit that removes the 'packages' prefix from the @load, then we can never use 2.5.1 anyway, it doesn't have the necessary fix in broctl.

if I depend on bro >= 2.5-140 I think it will complain that bro 2.5.1 is not new enough

But hypothetically, I think that's incorrect. 2.5.1 should satisfy that spec:

$ python
Python 3.6.2 (default, Jul 17 2017, 16:44:45) 
[GCC 4.2.1 Compatible Apple LLVM 8.1.0 (clang-802.0.42)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from semantic_version import *
>>> spec = Spec(">=2.5-140")
>>> version = Version.coerce("2.5.1")
>>> version in spec
True

but if I depend on 2.5.1, it won't allow master.

Yeah, I just posted more thoughts about the state of Bro's versioning in https://github.com/bro/package-manager/issues/15

I think probably in the future it should be rare to depend on patch-level releases, and certainly for 2.5.1, the state of things is messy enough to be not advisable.

@JustinAzoff
Copy link
Contributor

Did you see I had !=2.5.1 in the version requirement? i.e. If you choose to merge the commit that removes the 'packages' prefix from the @load, then we can never use 2.5.1 anyway, it doesn't have the necessary fix in broctl.

Ah, that makes sense.

I guess there's no great solution at the moment... either way breaks things for some people.

I think I'll leave this un-merged then for now, so that at least everything works as good as it can on a bro 2.5.1 install. Once a new release is out including the broctl fixes I'll update the requirements.

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

Successfully merging this pull request may close these issues.

2 participants