<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jul 13, 2016 at 9:27 PM, Anders Bertelrud <span dir="ltr"><<a href="mailto:anders@apple.com" target="_blank">anders@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div>Thanks for taking the initiative for this, Ankit. It's a very welcome improvement.</div><div><br></div><div>Comments inline.</div><span class=""><div><br></div>On 2016-07-12, at 11.15, Ankit Agarwal via swift-build-dev <<a href="mailto:swift-build-dev@swift.org" target="_blank">swift-build-dev@swift.org</a>> wrote:</span><div><span class=""><blockquote type="cite"><div dir="ltr"><div><p style="margin:15px 0px;font-family:Helvetica,arial,sans-serif;font-size:14px">To mark a target as exported/public I propose <code style="margin:0px 2px;padding:0px 5px;white-space:nowrap;border:1px solid rgb(234,234,234);background-color:rgb(248,248,248);border-top-left-radius:3px;border-top-right-radius:3px;border-bottom-right-radius:3px;border-bottom-left-radius:3px">PackageDescription</code>'s <code style="margin:0px 2px;padding:0px 5px;white-space:nowrap;border:1px solid rgb(234,234,234);background-color:rgb(248,248,248);border-top-left-radius:3px;border-top-right-radius:3px;border-bottom-right-radius:3px;border-bottom-left-radius:3px">Target</code> gains a <code style="margin:0px 2px;padding:0px 5px;white-space:nowrap;border:1px solid rgb(234,234,234);background-color:rgb(248,248,248);border-top-left-radius:3px;border-top-right-radius:3px;border-bottom-right-radius:3px;border-bottom-left-radius:3px">flags</code> property which would be a <code style="margin:0px 2px;padding:0px 5px;white-space:nowrap;border:1px solid rgb(234,234,234);background-color:rgb(248,248,248);border-top-left-radius:3px;border-top-right-radius:3px;border-bottom-right-radius:3px;border-bottom-left-radius:3px">Set</code> of the following <code style="margin:0px 2px;padding:0px 5px;white-space:nowrap;border:1px solid rgb(234,234,234);background-color:rgb(248,248,248);border-top-left-radius:3px;border-top-right-radius:3px;border-bottom-right-radius:3px;border-bottom-left-radius:3px">Flag</code> enum declared inside <code style="margin:0px 2px;padding:0px 5px;white-space:nowrap;border:1px solid rgb(234,234,234);background-color:rgb(248,248,248);border-top-left-radius:3px;border-top-right-radius:3px;border-bottom-right-radius:3px;border-bottom-left-radius:3px">Target</code> class:</p><pre style="margin-top:0.5em;margin-bottom:0.5em;background-color:rgb(245,242,240);border:1px solid rgb(204,204,204);font-size:13px;line-height:1.5;overflow:auto;padding:1em;border-top-left-radius:3px;border-top-right-radius:3px;border-bottom-right-radius:3px;border-bottom-left-radius:3px;font-family:Consolas,Monaco,'Andale Mono','Ubuntu Mono',monospace;direction:ltr;word-wrap:normal"><code style="margin:0px;padding:0px;border:none;background-color:transparent;border-top-left-radius:3px;border-top-right-radius:3px;border-bottom-right-radius:3px;border-bottom-left-radius:3px;font-family:Consolas,Monaco,'Andale Mono','Ubuntu Mono',monospace;direction:ltr;word-spacing:normal;word-wrap:normal;line-height:1.5"><span style="color:rgb(0,119,170)">public</span> <span style="color:rgb(0,119,170)">enum</span> <span style="color:rgb(102,153,0)">Flag</span> <span style="color:rgb(153,153,153)">{</span>
<span style="color:rgb(112,128,144)">/// Makes the target public or "exported" for other packages to use.</span>
<span style="color:rgb(0,119,170)">case</span> <span style="color:rgb(0,119,170)">public</span>
<span style="color:rgb(153,153,153)">}</span></code></pre><p style="margin:15px 0px;font-family:Helvetica,arial,sans-serif;font-size:14px">The <code style="margin:0px 2px;padding:0px 5px;white-space:nowrap;border:1px solid rgb(234,234,234);background-color:rgb(248,248,248);border-top-left-radius:3px;border-top-right-radius:3px;border-bottom-right-radius:3px;border-bottom-left-radius:3px">Flag</code> enum will be flexible in case we need to add more attributes in future as opposed to a boolean property to mark the public nature of the target.</p></div></div></blockquote></span><div>I would prefer that this be a boolean parameter rather than a generic `flags` parameter, since it makes the manifest read more naturally, and, importantly, is no less extensible than an enum. Additional parameters with default values can as easily be added as more enum cases can, and in either case, a manifest written to assume the existence of `public` will be equally incompatible with older versions of the package manager.</div><div><br></div>So, for example:</div><div><br></div><div><div><div dir="ltr"><div><pre style="margin-top:0.5em;margin-bottom:0.5em;background-color:rgb(245,242,240);border:1px solid rgb(204,204,204);font-size:13px;line-height:1.5;overflow:auto;padding:1em;border-top-left-radius:3px;border-top-right-radius:3px;border-bottom-right-radius:3px;border-bottom-left-radius:3px;font-family:Consolas,Monaco,'Andale Mono','Ubuntu Mono',monospace;direction:ltr;word-wrap:normal"><code style="margin:0px;padding:0px;border:none;border-top-left-radius:3px;border-top-right-radius:3px;border-bottom-right-radius:3px;border-bottom-left-radius:3px;font-family:Consolas,Monaco,'Andale Mono','Ubuntu Mono',monospace;direction:ltr;word-spacing:normal;word-wrap:normal;line-height:1.5"><span class=""><span style="color:rgb(0,119,170)">let</span> package <span style="color:rgb(166,127,89);background-color:rgba(255,255,255,0.498039)">=</span> <span style="color:rgb(221,74,104)">Package</span><span style="color:rgb(153,153,153)">(</span>
name<span style="color:rgb(153,153,153)">:</span> <span style="color:rgb(102,153,0)">"FooLibrary"</span><span style="color:rgb(153,153,153)">,</span>
targets<span style="color:rgb(153,153,153)">:</span> <span style="color:rgb(153,153,153)">[</span></span>
<span style="color:rgb(221,74,104)">Target</span><span style="color:rgb(153,153,153)">(</span>name<span style="color:rgb(153,153,153)">:</span> <span style="color:rgb(102,153,0)">"FooLibrary"</span><span style="color:rgb(153,153,153)">,</span> public: <span style="color:rgb(0,119,170);word-spacing:normal">true</span><span style="color:rgb(153,153,153)">)</span><span style="color:rgb(153,153,153)">,</span><span class="">
<span style="color:rgb(221,74,104)">Target</span><span style="color:rgb(153,153,153)">(</span>name<span style="color:rgb(153,153,153)">:</span> <span style="color:rgb(102,153,0)">"SampleCLI"</span><span style="color:rgb(153,153,153)">,</span> dependencies<span style="color:rgb(153,153,153)">:</span> <span style="color:rgb(153,153,153)">[</span><span style="color:rgb(102,153,0)">"FooLibrary"</span><span style="color:rgb(153,153,153)">]</span><span style="color:rgb(153,153,153)">)</span><span style="color:rgb(153,153,153)">,</span>
<span style="color:rgb(153,153,153)">]</span><span style="color:rgb(153,153,153)">)</span></span></code></pre></div></div></div><blockquote type="cite"><div><div dir="ltr"><div><p style="margin:15px 0px;font-family:Helvetica,arial,sans-serif;font-size:14px">We can keep some obvious defaults for targets which can be implicitly public for e.g. </p><span class=""><ol style="margin:15px 0px;padding-left:30px;font-family:Helvetica,arial,sans-serif;font-size:14px"><li style="margin:0px">Package has only one target.</li><li style="margin:0px">Target with same name as package.</li></ol></span></div></div></div></blockquote>I'm a bit wary of magic here. I think it would be clearer to have the manifest declare what is public and what is not. With magic naming conventions it's too easy to accidentally change semantics just by renaming a target.</div></div></blockquote><div><br></div><div>I agree that we should avoid too much magic, I think you missed the sentence below those examples in the proposal where I mentioned that we should avoid those instead. However as Daniel mentioned we don't want to overcomplicate the manifest for simple packages and for beginners, keeping that in mind we updated the proposed solution which you can find on the proposal link (<a href="https://github.com/aciidb0mb3r/swift-evolution/blob/swiftpm-module-access-control/proposals/xxxx-swiftpm-target-access-control.md">https://github.com/aciidb0mb3r/swift-evolution/blob/swiftpm-module-access-control/proposals/xxxx-swiftpm-target-access-control.md</a>)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><span class=""><blockquote type="cite"><div dir="ltr"><div><p style="margin:15px 0px;font-family:Helvetica,arial,sans-serif;font-size:14px">I propose that enum <code style="margin:0px 2px;padding:0px 5px;white-space:nowrap;border:1px solid rgb(234,234,234);background-color:rgb(248,248,248);border-top-left-radius:3px;border-top-right-radius:3px;border-bottom-right-radius:3px;border-bottom-left-radius:3px">Target.Dependency</code> gains a new case <code style="margin:0px 2px;padding:0px 5px;white-space:nowrap;border:1px solid rgb(234,234,234);background-color:rgb(248,248,248);border-top-left-radius:3px;border-top-right-radius:3px;border-bottom-right-radius:3px;border-bottom-left-radius:3px">External(package: String, target: String)</code> to declare dependency on an external package's target.</p></div></div></blockquote></span><div>Since it's the same fundamental kind of dependency in either case, would it be better to have `package` be an optional parameter to Target?</div><div><br></div><div>So that `Target(name: "Foo")` is local but `Target(name: "Foo", package: "Bar")` external? That would seem more logical.</div></div><span class=""><font color="#888888"><br></font></span></div></blockquote></div><br>I like `Target(name: "Foo", package: "Bar")` but I prefer package name is stated before the target name `Target(package: "Foo", name: "Bar")` but then this gets weird and `Target(package:target)` is also weird so I chose `External(package:target)` instead. However if people prefer `Target(name:package)` more then I am fine with it.</div><div class="gmail_extra"><div><br></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature">Ankit<br><br></div>
</div></div>