<p>An OS that is niche to the point that I haven't even heard of it is hardly more justifiable than a dead one. And it's not just a niche OS or a dead OS but it's also a non-POSIX, decidedly non-unix OS, so it's quite a leap we're talking about here.</p>
<p>And we've just been considering bumping the system requirement to POSIX.1-2008 (from 2001) to make certain things simpler/possible.</p>
<p>That said, there are a number of things in this patch that could be merged if done right. Some examples:</p>
<ul>
<li>The IMA plugin should not be built at all if <sys/xattr.h> and lxsetattr() are not present on the system. That check belongs to configure.ac + Makefiles and has nothing at all to do with OS/2, it's plain correctness.</li>
<li>Similarly for ctermid(), test for the existence of the function, not OS.</li>
<li>build/pack.c already includes <sys/wait.h>. If the order is wrong then change that instead of yet more ifdefs</li>
<li>__python_path hardcoding /usr/lib is wrong anyway, but so is %{prefix} because rpm's prefix and python's prefix don't necessarily have anything to do with each other, it should discover the python path by some other means</li>
<li>The added error check in doScript() for child == -1 would be correct for fork() too. I also wonder why is spawnvp() used there if everything else seems to be happy with fork()?</li>
<li>Line- and path-separator issues could be handled centrally (configure.ac / system.h macro or such) without making it any worse for anybody</li>
</ul>
<p>Quite possibly something else too, I didn't look <em>that</em> carefully. In general, the way to add portability is not to sprinkle ifdef's but add abstraction.</p>
<p>tools/os2deps.c is missing in that patch so no idea what it does, but the invocation through a script helper looks a bit strange, and this magic pattern looks overly broad, I'd expect there to be some further identification in that string:<br>
+%__os2_magic           ^(32|64)-bit.*$</p>
<p>In any case it looks like that's dealing with os2 <em>executables</em> so a better name would be something less broad than "os2".</p>

<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/rpm-software-management/rpm/pull/260#issuecomment-339914006">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ANb80wo0WR5i7p_9e4a1iYp873bO1APYks5swZpcgaJpZM4Oc5Yj">mute the thread</a>.<img alt="" height="1" src="https://github.com/notifications/beacon/ANb809zyQvkmcgTsgZp1Oj8knvU2yDQfks5swZpcgaJpZM4Oc5Yj.gif" width="1" /></p>
<div itemscope itemtype="http://schema.org/EmailMessage">
<div itemprop="action" itemscope itemtype="http://schema.org/ViewAction">
  <link itemprop="url" href="https://github.com/rpm-software-management/rpm/pull/260#issuecomment-339914006"></link>
  <meta itemprop="name" content="View Pull Request"></meta>
</div>
<meta itemprop="description" content="View this Pull Request on GitHub"></meta>
</div>

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/rpm-software-management/rpm","title":"rpm-software-management/rpm","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/rpm-software-management/rpm"}},"updates":{"snippets":[{"icon":"PERSON","message":"@pmatilai in #260: An OS that is niche to the point that I haven't even heard of it is hardly more justifiable than a dead one. And it's not just a niche OS or a dead OS but it's also a non-POSIX, decidedly non-unix OS, so it's quite a leap we're talking about here.\r\n\r\nAnd we've just been considering bumping the system requirement to POSIX.1-2008 (from 2001) to make certain things simpler/possible.\r\n\r\nThat said, there are a number of things in this patch that could be merged if done right. Some examples:\r\n* The IMA plugin should not be built at all if \u003csys/xattr.h\u003e and lxsetattr() are not present on the system. That check belongs to configure.ac + Makefiles and has nothing at all to do with OS/2, it's plain correctness.\r\n* Similarly for ctermid(), test for the existence of the function, not OS.\r\n* build/pack.c already includes \u003csys/wait.h\u003e. If the order is wrong then change that instead of yet more ifdefs\r\n* __python_path hardcoding /usr/lib is wrong anyway, but so is %{prefix} because rpm's prefix and python's prefix don't necessarily have anything to do with each other, it should discover the python path by some other means\r\n* The added error check in doScript() for child == -1 would be correct for fork() too. I also wonder why is spawnvp() used there if everything else seems to be happy with fork()?\r\n* Line- and path-separator issues could be handled centrally (configure.ac / system.h macro or such) without making it any worse for anybody\r\n\r\nQuite possibly something else too, I didn't look *that* carefully. In general, the way to add portability is not to sprinkle ifdef's but add abstraction.\r\n\r\ntools/os2deps.c is missing in that patch so no idea what it does, but the invocation through a script helper looks a bit strange, and this magic pattern looks overly broad, I'd expect there to be some further identification in that string:\r\n+%__os2_magic           ^(32|64)-bit.*$\r\n\r\nIn any case it looks like that's dealing with os2 *executables* so a better name would be something less broad than \"os2\"."}],"action":{"name":"View Pull Request","url":"https://github.com/rpm-software-management/rpm/pull/260#issuecomment-339914006"}}}</script>