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

Using SCons tools #977

Merged
merged 5 commits into from
Dec 26, 2021
Merged

Using SCons tools #977

merged 5 commits into from
Dec 26, 2021

Conversation

Amaras
Copy link
Member

@Amaras Amaras commented Dec 22, 2021

This is a PR to remove the language builders from the main SConstruct file, and put them into their own tools.

I confirm that everything compiles on my machine, but GH actions will prove me right or wrong on general Ubuntu, and I have no idea what's gonna happen on Windows.

This was inspired by my thought process on #976, and it gives us a structure for future compiled languages.

I did not touch the Copier builder, and there are probably things I could remove for uniformity in rust_SConscript.

Apart from that, if you have any remark, feel free to leave them here.

@Amaras Amaras added the SCons For SCons-related matters label Dec 22, 2021
@PeanutbutterWarrior
Copy link
Contributor

From the SCons docs:

At the moment, user-added tools do not automatically have their exists function called. As a result, it is recommended that the generate function be defensively coded - that is, do not rely on any necessary existence checks already having been performed. This is expected to be a temporary limitation, and the exists function should still be provided.

I'm not sure how this would best be remedied. Immediately returning from the generate functions isn't viable, as the build script sees that as a success and crashes SCons in it's entirety when it tries to build that language with an AttributeError.

I think we should add some sentry value instead of the builder to avoid the AttributeError, and then have the SConscripts check against this value and fail immediately and print some message. I would prefer it fail when SCons tries to build the tool, but I can't find how we would achieve this.

We could have the languages dictionary accessible through the Environment, and remove languages that can't be built from it when the generate function gets called, but I fear this would end up hard to debug and obscure.

@PeanutbutterWarrior
Copy link
Contributor

I'm also not a massive fan of having two directories just for SCons plumbing, but it's just a minor gripe.

@Amaras
Copy link
Member Author

Amaras commented Dec 23, 2021

I'm also not a massive fan of having two directories just for SCons plumbing, but it's just a minor gripe.

This could be remedied simply by putting both the builders and sconscripts directories in a scons directory, and changing toolpath=['builders'] to toolpath=['scons/tools'] or something like that

I'm not sure how this would best be remedied. Immediately returning from the generate functions isn't viable, as the build script sees that as a success and crashes SCons in it's entirety when it tries to build that language with an AttributeError.

This is something that is remedied by the Kotlin tool in scons-contrib repository, something like this:

class ToolKotlinWarning(SCons.Warnings.SConsWarning):
    pass


class KotlinNotFound(ToolKotlinWarning):
    pass


SCons.Warnings.enableWarningClass(ToolKotlinWarning)


def _detect(env):
    """ Try to detect the kotlinc binary """
    try:
        return env["kotlinc"]
    except KeyError:
        pass

    kotlin = env.WhereIs("kotlinc")
    if kotlin:
        return kotlin

    raise SCons.Errors.StopError(KotlinNotFound, "Could not detect kotlinc executable")
    return None

with generate calling env['KOTLINC'] = _detect(env). It is probably a good enough solution, but potentially prevents building entirely, so I'll try that.

@Amaras
Copy link
Member Author

Amaras commented Dec 23, 2021

Following discussion on Discord, @PeanutbutterWarrior and I converged to this version.
The only thing left is to decide whether or not we want to have a single directory for SCons thingies, or if we keep two.

@Amaras Amaras merged commit b17f07b into algorithm-archivists:main Dec 26, 2021
@Amaras Amaras deleted the use_scons_tools branch December 26, 2021 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SCons For SCons-related matters
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants