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

Make ZSS build fail when RC != 0 #188

Merged
merged 2 commits into from
May 29, 2020

Conversation

ifakhrutdinov
Copy link
Contributor

Overview

Currently, if a warning message is produced by the compiler, the build process is considered successful. This is often dangerous as warnings may mean passing the wrong type or redefinition of a #define, which should be considered bugs. The pull-requests makes the build process stricter - any warning messages will now lead to a failing build.

Changes

  • Make sure there are no warning messages in the current build
    • Update deps to remove the httpfileservice.c warning message, and pick up a minor type fix
    • Ensure side-deck file/SYSDEFSD DD by adding the dll option to the linker
  • Adjust the compiler env variable controlling the severity
  • Ensure no ZSS binary is created if RC != 0

* Make sure there are no warning messages in the current build
  * Update deps to remove the httpfileservice.c warning message
  * Ensure side-deck file/SYSDEFSD DD by adding the dll option to the linker
* Set the compiler env variable controlling the severity

Signed-off-by: Irek Fakhrutdinov <[email protected]>
@ifakhrutdinov ifakhrutdinov added the enhancement New feature or request label May 29, 2020
@ifakhrutdinov ifakhrutdinov self-assigned this May 29, 2020
mkdir -p "${WORKING_DIR}/tmp-zss" && cd "$_"

IFS='.' read -r major minor micro < "${ZSS}/version.txt"
date_stamp=$(date +%Y%m%d)
echo "Version: $major.$minor.$micro"
echo "Date stamp: $date_stamp"

c89 \
export _C89_ACCEPTABLE_RC=0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this isnt doing anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should tell c89 to produce a non-zero RC if any of its internal stages returns a non-zero RC. Otherwise, a warning message will still only be a message and c89 will return 0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, so it's not for the script, its for xlc. makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it's a env variable controlling the compiler.

@@ -62,7 +70,7 @@ c89 \
-DAPF_AUTHORIZED=0 \
-Wc,dll,expo,langlvl\(extc99\),gonum,goff,hgpr,roconst,ASM,asmlib\('CEE.SCEEMAC','SYS1.MACLIB','SYS1.MODGEN'\) \
-Wc,agg,exp,list\(\),so\(\),off,xref \
-Wl,ac=1 \
-Wl,ac=1,dll \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you give a short description about the difference here? I see it is a supported flag but I am curious about what was wrong when we only had dll for -Wc, but not -Wl

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the manual, the dll compiler options tell the compiler that some symbols will come from a dll, the dll linker option tells the linker to produce a side file and a proper dll.

I've just tested and the build works fine without the compiler dll option. However, the build will produce a warning if there is no linker dll option. I think it's because, we still have the export all option, which makes c89 create a binary with exported symbols, but a missing dll option causes the linker not to allocate the SYSDEFSD (side-deck DD) and produce a warning message.

When you bind code with exported symbols, you should specify the DLL
binder option (-W l,dll).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the things this does is generate the side file with every single function. We don't really want that since it is a form of API documentation and we only want a subset of those functions to be externally used in case we change something. So, the .x file is going to be discarded anyway. This PR was an attempt to make an official one: #30
So, is this still better even if we do not use the .x file that gets generated?

Copy link
Contributor Author

@ifakhrutdinov ifakhrutdinov May 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, is this still better even if we do not use the .x file that gets generated?

This is just to make the linker stop producing that warning message. That PR is still relevant. We need to select a subset of the functions we want to export. That is, right now it's "export all" later it'll be specific pragmas, but we'll have to use -Wl,dll in either case.

Copy link
Contributor

@daveyc daveyc May 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's acceptable to me. I don't know of any way to stop generating a side file with -Wc,dll. It's a lot of work to specifically single out what functions should be exported using #pragma directives.

Copy link
Contributor

@daveyc daveyc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will _C89_ACCEPTABLE_RC work with C99 code?

@ifakhrutdinov
Copy link
Contributor Author

Will _C89_ACCEPTABLE_RC work with C99 code?

I don't think this is for a certain level of code, this is for the c89 binary, which is used in build_zss.sh.

@1000TurquoisePogs 1000TurquoisePogs merged commit 47338b3 into zowe:staging May 29, 2020
@ifakhrutdinov ifakhrutdinov deleted the feature/stricter-build branch May 29, 2020 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants